From 9c85b8e8c1b30a432cdbabd599f8cf7e5c233123 Mon Sep 17 00:00:00 2001 From: Cedric Carrard Date: Wed, 20 Mar 2019 13:47:57 +0100 Subject: [PATCH] fix redirect loop --- helpdesk/decorators.py | 41 +++++++++- helpdesk/tests/helpers.py | 12 ++- helpdesk/tests/test_navigation.py | 49 +++++++++++- helpdesk/tests/test_savequery.py | 4 +- helpdesk/views/staff.py | 127 +++++++----------------------- 5 files changed, 126 insertions(+), 107 deletions(-) diff --git a/helpdesk/decorators.py b/helpdesk/decorators.py index 47dc5911..3ef8dfec 100644 --- a/helpdesk/decorators.py +++ b/helpdesk/decorators.py @@ -1,9 +1,12 @@ from functools import wraps -from django.urls import reverse -from django.http import HttpResponseRedirect, Http404 +from django.core.exceptions import PermissionDenied +from django.http import Http404 +from django.shortcuts import redirect + from django.utils.decorators import available_attrs + from helpdesk import settings as helpdesk_settings @@ -15,9 +18,41 @@ def protect_view(view_func): @wraps(view_func, assigned=available_attrs(view_func)) def _wrapped_view(request, *args, **kwargs): if not request.user.is_authenticated and helpdesk_settings.HELPDESK_REDIRECT_TO_LOGIN_BY_DEFAULT: - return HttpResponseRedirect(reverse('helpdesk:login')) + return redirect('helpdesk:login') elif not request.user.is_authenticated and helpdesk_settings.HELPDESK_ANON_ACCESS_RAISES_404: raise Http404 return view_func(request, *args, **kwargs) return _wrapped_view + + +def staff_member_required(view_func): + """ + Decorator for staff member the views checking user, redirecting + to the log-in page if necessary or returning 403 + """ + @wraps(view_func, assigned=available_attrs(view_func)) + def _wrapped_view(request, *args, **kwargs): + if not request.user.is_authenticated and not request.user.is_active: + return redirect('helpdesk:login') + if not helpdesk_settings.HELPDESK_ALLOW_NON_STAFF_TICKET_UPDATE and not request.user.is_staff: + raise PermissionDenied() + return view_func(request, *args, **kwargs) + + return _wrapped_view + + +def superuser_required(view_func): + """ + Decorator for superuser member the views checking user, redirecting + to the log-in page if necessary or returning 403 + """ + @wraps(view_func, assigned=available_attrs(view_func)) + def _wrapped_view(request, *args, **kwargs): + if not request.user.is_authenticated and not request.user.is_active: + return redirect('helpdesk:login') + if not request.user.is_superuser: + raise PermissionDenied() + return view_func(request, *args, **kwargs) + + return _wrapped_view diff --git a/helpdesk/tests/helpers.py b/helpdesk/tests/helpers.py index 5d6c6bfc..39172978 100644 --- a/helpdesk/tests/helpers.py +++ b/helpdesk/tests/helpers.py @@ -5,12 +5,18 @@ from django.contrib.auth import get_user_model User = get_user_model() -def get_staff_user(username='helpdesk.staff', password='password'): +def get_user(username='helpdesk.staff', + password='password', + is_staff=False, + is_superuser=False): try: user = User.objects.get(username=username) except User.DoesNotExist: - user = User.objects.create_user(username=username, password=password, email='staff@example.com') - user.is_staff = True + user = User.objects.create_user(username=username, + password=password, + email='%s@example.com' % username) + user.is_staff = is_staff + user.is_superuser = is_superuser user.save() else: user.set_password(password) diff --git a/helpdesk/tests/test_navigation.py b/helpdesk/tests/test_navigation.py index 18e2fbfa..8da9e8d9 100644 --- a/helpdesk/tests/test_navigation.py +++ b/helpdesk/tests/test_navigation.py @@ -3,7 +3,7 @@ from django.urls import reverse from django.test import TestCase from helpdesk.models import KBCategory -from helpdesk.tests.helpers import get_staff_user, reload_urlconf +from helpdesk.tests.helpers import get_user, reload_urlconf class TestKBDisabled(TestCase): @@ -26,7 +26,7 @@ class TestKBDisabled(TestCase): """Test proper rendering of navigation.html by accessing the dashboard""" from django.urls import NoReverseMatch - self.client.login(username=get_staff_user().get_username(), password='password') + self.client.login(username=get_user(is_staff=True).get_username(), password='password') self.assertRaises(NoReverseMatch, reverse, 'helpdesk:kb_index') try: response = self.client.get(reverse('helpdesk:dashboard')) @@ -45,3 +45,48 @@ class TestKBDisabled(TestCase): response = self.client.get(reverse('helpdesk:home')) self.assertEqual(response.status_code, 200) self.assertTemplateUsed(response, 'helpdesk/public_homepage.html') + + +class TestDecorator(TestCase): + + def test_staff_member_restrictions(self): + user = get_user(username='helpdesk.user', + password='password') + + self.client.login(username=user.get_username(), + password='password') + response = self.client.get(reverse('helpdesk:list')) + self.assertEqual(response.status_code, 403) + + def test_staff_member_access(self): + user = get_user(username='helpdesk.user', + password='password', + is_staff=True) + + self.client.login(username=user.get_username(), + password='password') + response = self.client.get(reverse('helpdesk:list')) + self.assertEqual(response.status_code, 200) + self.assertTemplateUsed(response, 'helpdesk/ticket_list.html') + + def test_superuser_member_restrictions(self): + user = get_user(username='helpdesk.superuser', + password='password', + is_staff=True) + + self.client.login(username=user.get_username(), + password='password') + response = self.client.get(reverse('helpdesk:email_ignore')) + self.assertEqual(response.status_code, 403) + + def test_superuser_member_access(self): + user = get_user(username='helpdesk.superuser', + password='password', + is_staff=True, + is_superuser=True) + + self.client.login(username=user.get_username(), + password='password') + response = self.client.get(reverse('helpdesk:email_ignore')) + self.assertEqual(response.status_code, 200) + self.assertTemplateUsed(response, 'helpdesk/email_ignore_list.html') diff --git a/helpdesk/tests/test_savequery.py b/helpdesk/tests/test_savequery.py index 8838b597..5916782b 100644 --- a/helpdesk/tests/test_savequery.py +++ b/helpdesk/tests/test_savequery.py @@ -2,7 +2,7 @@ from django.urls import reverse from django.test import TestCase from helpdesk.models import Queue -from helpdesk.tests.helpers import get_staff_user +from helpdesk.tests.helpers import get_user class TestSavingSharedQuery(TestCase): @@ -14,7 +14,7 @@ class TestSavingSharedQuery(TestCase): def test_cansavequery(self): """Can a query be saved""" url = reverse('helpdesk:savequery') - self.client.login(username=get_staff_user().get_username(), + self.client.login(username=get_user(is_staff=True).get_username(), password='password') response = self.client.post( url, diff --git a/helpdesk/views/staff.py b/helpdesk/views/staff.py index fb6ead3e..ff496156 100644 --- a/helpdesk/views/staff.py +++ b/helpdesk/views/staff.py @@ -13,10 +13,8 @@ import re from django import VERSION as DJANGO_VERSION from django.conf import settings from django.contrib.auth import get_user_model -from django.contrib.auth.decorators import user_passes_test from django.urls import reverse from django.core.exceptions import ValidationError, PermissionDenied -from django.db import connection from django.db.models import Q from django.http import HttpResponseRedirect, Http404, HttpResponse from django.shortcuts import render, get_object_or_404 @@ -32,8 +30,9 @@ from helpdesk.forms import ( TicketForm, UserSettingsForm, EmailIgnoreForm, EditTicketForm, TicketCCForm, TicketCCEmailForm, TicketCCUserForm, EditFollowUpForm, TicketDependencyForm ) +from helpdesk.decorators import staff_member_required, superuser_required from helpdesk.lib import ( - send_templated_mail, query_to_dict, apply_query, safe_template_context, + send_templated_mail, apply_query, safe_template_context, process_attachments, queue_template_context, ) from helpdesk.models import ( @@ -42,22 +41,10 @@ from helpdesk.models import ( ) from helpdesk import settings as helpdesk_settings + User = get_user_model() -if helpdesk_settings.HELPDESK_ALLOW_NON_STAFF_TICKET_UPDATE: - # treat 'normal' users like 'staff' - staff_member_required = user_passes_test( - lambda u: u.is_authenticated and u.is_active) -else: - staff_member_required = user_passes_test( - lambda u: u.is_authenticated and u.is_active and u.is_staff) - - -superuser_required = user_passes_test( - lambda u: u.is_authenticated and u.is_active and u.is_superuser) - - def _get_user_queues(user): """Return the list of Queues the user can access. @@ -97,6 +84,7 @@ def _is_my_ticket(user, ticket): return False +@staff_member_required def dashboard(request): """ A quick summary overview for users: A list of their own tickets, a table @@ -162,9 +150,7 @@ def dashboard(request): }) -dashboard = staff_member_required(dashboard) - - +@staff_member_required def delete_ticket(request, ticket_id): ticket = get_object_or_404(Ticket, id=ticket_id) if not _has_access_to_queue(request.user, ticket.queue): @@ -181,9 +167,7 @@ def delete_ticket(request, ticket_id): return HttpResponseRedirect(reverse('helpdesk:home')) -delete_ticket = staff_member_required(delete_ticket) - - +@staff_member_required def followup_edit(request, ticket_id, followup_id): """Edit followup options with an ability to change the ticket.""" followup = get_object_or_404(FollowUp, id=followup_id) @@ -236,9 +220,7 @@ def followup_edit(request, ticket_id, followup_id): return HttpResponseRedirect(reverse('helpdesk:view', args=[ticket.id])) -followup_edit = staff_member_required(followup_edit) - - +@staff_member_required def followup_delete(request, ticket_id, followup_id): """followup delete for superuser""" @@ -251,9 +233,7 @@ def followup_delete(request, ticket_id, followup_id): return HttpResponseRedirect(reverse('helpdesk:view', args=[ticket.id])) -followup_delete = staff_member_required(followup_delete) - - +@staff_member_required def view_ticket(request, ticket_id): ticket = get_object_or_404(Ticket, id=ticket_id) if not _has_access_to_queue(request.user, ticket.queue): @@ -323,9 +303,6 @@ def view_ticket(request, ticket_id): }) -view_ticket = staff_member_required(view_ticket) - - def return_ticketccstring_and_show_subscribe(user, ticket): """used in view_ticket() and followup_edit()""" # create the ticketcc_string and check whether current user is already @@ -666,6 +643,7 @@ def return_to_ticket(user, helpdesk_settings, ticket): return HttpResponseRedirect(ticket.ticket_url) +@staff_member_required def mass_update(request): tickets = request.POST.getlist('ticket_id') action = request.POST.get('action', None) @@ -781,9 +759,7 @@ def mass_update(request): return HttpResponseRedirect(reverse('helpdesk:list')) -mass_update = staff_member_required(mass_update) - - +@staff_member_required def ticket_list(request): context = {} @@ -970,9 +946,7 @@ def ticket_list(request): )) -ticket_list = staff_member_required(ticket_list) - - +@staff_member_required def edit_ticket(request, ticket_id): ticket = get_object_or_404(Ticket, id=ticket_id) if not _has_access_to_queue(request.user, ticket.queue): @@ -991,9 +965,7 @@ def edit_ticket(request, ticket_id): return render(request, 'helpdesk/edit_ticket.html', {'form': form}) -edit_ticket = staff_member_required(edit_ticket) - - +@staff_member_required def create_ticket(request): if helpdesk_settings.HELPDESK_STAFF_ONLY_TICKET_OWNERS: assignable_users = User.objects.filter(is_active=True, is_staff=True).order_by(User.USERNAME_FIELD) @@ -1030,9 +1002,7 @@ def create_ticket(request): return render(request, 'helpdesk/create_ticket.html', {'form': form}) -create_ticket = staff_member_required(create_ticket) - - +@staff_member_required def raw_details(request, type): # TODO: This currently only supports spewing out 'PreSetReply' objects, # in the future it needs to be expanded to include other items. All it @@ -1051,9 +1021,7 @@ def raw_details(request, type): raise Http404 -raw_details = staff_member_required(raw_details) - - +@staff_member_required def hold_ticket(request, ticket_id, unhold=False): ticket = get_object_or_404(Ticket, id=ticket_id) if not _has_access_to_queue(request.user, ticket.queue): @@ -1082,23 +1050,17 @@ def hold_ticket(request, ticket_id, unhold=False): return HttpResponseRedirect(ticket.get_absolute_url()) -hold_ticket = staff_member_required(hold_ticket) - - +@staff_member_required def unhold_ticket(request, ticket_id): return hold_ticket(request, ticket_id, unhold=True) -unhold_ticket = staff_member_required(unhold_ticket) - - +@staff_member_required def rss_list(request): return render(request, 'helpdesk/rss_list.html', {'queues': Queue.objects.all()}) -rss_list = staff_member_required(rss_list) - - +@staff_member_required def report_index(request): number_tickets = Ticket.objects.all().count() saved_query = request.GET.get('saved_query', None) @@ -1133,9 +1095,7 @@ def report_index(request): }) -report_index = staff_member_required(report_index) - - +@staff_member_required def run_report(request, report): if Ticket.objects.all().count() == 0 or report not in ( 'queuemonth', 'usermonth', 'queuestatus', 'queuepriority', 'userstatus', @@ -1339,9 +1299,7 @@ def run_report(request, report): }) -run_report = staff_member_required(run_report) - - +@staff_member_required def save_query(request): title = request.POST.get('title', None) shared = request.POST.get('shared', False) @@ -1358,9 +1316,7 @@ def save_query(request): return HttpResponseRedirect('%s?saved_query=%s' % (reverse('helpdesk:list'), query.id)) -save_query = staff_member_required(save_query) - - +@staff_member_required def delete_saved_query(request, id): query = get_object_or_404(SavedSearch, id=id, user=request.user) @@ -1371,9 +1327,7 @@ def delete_saved_query(request, id): return render(request, 'helpdesk/confirm_delete_saved_query.html', {'query': query}) -delete_saved_query = staff_member_required(delete_saved_query) - - +@staff_member_required def user_settings(request): s = request.user.usersettings_helpdesk if request.POST: @@ -1387,18 +1341,14 @@ def user_settings(request): return render(request, 'helpdesk/user_settings.html', {'form': form}) -user_settings = staff_member_required(user_settings) - - +@superuser_required def email_ignore(request): return render(request, 'helpdesk/email_ignore_list.html', { 'ignore_list': IgnoreEmail.objects.all(), }) -email_ignore = superuser_required(email_ignore) - - +@superuser_required def email_ignore_add(request): if request.method == 'POST': form = EmailIgnoreForm(request.POST) @@ -1411,9 +1361,7 @@ def email_ignore_add(request): return render(request, 'helpdesk/email_ignore_add.html', {'form': form}) -email_ignore_add = superuser_required(email_ignore_add) - - +@superuser_required def email_ignore_del(request, id): ignore = get_object_or_404(IgnoreEmail, id=id) if request.method == 'POST': @@ -1423,9 +1371,7 @@ def email_ignore_del(request, id): return render(request, 'helpdesk/email_ignore_del.html', {'ignore': ignore}) -email_ignore_del = superuser_required(email_ignore_del) - - +@staff_member_required def ticket_cc(request, ticket_id): ticket = get_object_or_404(Ticket, id=ticket_id) if not _has_access_to_queue(request.user, ticket.queue): @@ -1440,9 +1386,7 @@ def ticket_cc(request, ticket_id): }) -ticket_cc = staff_member_required(ticket_cc) - - +@staff_member_required def ticket_cc_add(request, ticket_id): ticket = get_object_or_404(Ticket, id=ticket_id) if not _has_access_to_queue(request.user, ticket.queue): @@ -1468,9 +1412,7 @@ def ticket_cc_add(request, ticket_id): }) -ticket_cc_add = staff_member_required(ticket_cc_add) - - +@staff_member_required def ticket_cc_del(request, ticket_id, cc_id): cc = get_object_or_404(TicketCC, ticket__id=ticket_id, id=cc_id) @@ -1481,9 +1423,7 @@ def ticket_cc_del(request, ticket_id, cc_id): return render(request, 'helpdesk/ticket_cc_del.html', {'cc': cc}) -ticket_cc_del = staff_member_required(ticket_cc_del) - - +@staff_member_required def ticket_dependency_add(request, ticket_id): ticket = get_object_or_404(Ticket, id=ticket_id) if not _has_access_to_queue(request.user, ticket.queue): @@ -1506,9 +1446,7 @@ def ticket_dependency_add(request, ticket_id): }) -ticket_dependency_add = staff_member_required(ticket_dependency_add) - - +@staff_member_required def ticket_dependency_del(request, ticket_id, dependency_id): dependency = get_object_or_404(TicketDependency, ticket__id=ticket_id, id=dependency_id) if request.method == 'POST': @@ -1517,9 +1455,7 @@ def ticket_dependency_del(request, ticket_id, dependency_id): return render(request, 'helpdesk/ticket_dependency_del.html', {'dependency': dependency}) -ticket_dependency_del = staff_member_required(ticket_dependency_del) - - +@staff_member_required def attachment_del(request, ticket_id, attachment_id): ticket = get_object_or_404(Ticket, id=ticket_id) if not _has_access_to_queue(request.user, ticket.queue): @@ -1537,9 +1473,6 @@ def attachment_del(request, ticket_id, attachment_id): }) -attachment_del = staff_member_required(attachment_del) - - def calc_average_nbr_days_until_ticket_resolved(Tickets): nbr_closed_tickets = len(Tickets) days_per_ticket = 0