Merge branch 'main' into hotfix/followup-card-colors

This commit is contained in:
Christopher Broderick
2026-09-15 09:23:44 +01:00
committed by GitHub
14 changed files with 793 additions and 124 deletions

View File

@@ -5,6 +5,11 @@ on:
push:
branches:
- main
# Maintenance lines, 2.4.x and whatever follows. A push to one of these is
# a release candidate and deserves the same matrix as main, and it is the
# only chance it gets: these branches never reach main through a pull
# request, so nothing else would ever run the tests on them.
- "*.x"
concurrency:
group: check-${{ github.ref }}

19
AUTHORS
View File

@@ -1,12 +1,11 @@
django-helpdesk was originally written by Ross Poulton. After nearly nine
django-helpdesk was originally written by Ross Poulton (@rossp). After nearly nine
years under his guidance, the project graduated to its own eponymous
organization, and is currently maintained by:
Christopher Broderick (@uhurusurfa)
(@Benbb96)
David Vadnais (@DavidVadnais)
Alex Seeholzer (@flinz)
Jonathan Barratt (@reduxionist)
Gurupratap Matharu (@gurupratap-matharu)
Since publishing the code a number of people have made some fantastic
improvements and provided bug fixes and updates as the Django codebase
@@ -14,15 +13,21 @@ has moved on and caused small portions of this application to break.
To these people, and many more, our sincere thanks:
Alex Barcelo
Andreas Kotowicz
Christopher Broderick
Alex Barcelo (@alexbarcelo)
Alex Seeholzer (@flinz --> @seeholza)
Andreas Kotowicz (@kotowicz)
Christopher Broderick (@uhurusurfa)
Chris Etcp
Daryl Egarr
David Clymer
Garret Wassermann (@gwasser)
Jonathan Barratt (@reduxionist)
Loe Spee
Martin Whitehouse (@martin-marty)
Maxim Litnitskiy
Nikolay Panov
Stefano Brentegani
Sam Splunks (@samsplunks)
Stefano Brentegani (@brente)
Timothy Hobbs (@timthelion)
(@tkm84)
Tony Zhu

View File

@@ -3,11 +3,11 @@
"packageManager": "yarn@4.9.4",
"dependencies": {
"bootstrap": "5.3.8",
"datatables.net": "^3.0.3",
"datatables.net-bs5": "^3.0.3",
"datatables.net-buttons": "^4.0.1",
"datatables.net-buttons-bs5": "^4.0.2",
"datatables.net-buttons-dt": "^4.0.1",
"datatables.net": "^3.0.4",
"datatables.net-bs5": "^3.0.4",
"datatables.net-buttons": "^4.0.3",
"datatables.net-buttons-bs5": "^4.0.3",
"datatables.net-buttons-dt": "^4.0.3",
"fontawesome-free": "npm:@fortawesome/fontawesome-free@^6.7.2",
"jquery": "4.0.0",
"jquery-ui": "1.14.2",

View File

@@ -1,6 +1,6 @@
[project]
name = "django-helpdesk"
version = "2.4.0"
version = "2.5.0"
description = "Django-powered ticket tracker for your helpdesk"
authors = [
{name = "Ross Poulton", email = "ross@rossp.org"},

View File

@@ -46,6 +46,7 @@ from helpdesk.settings import (
HELPDESK_SHOW_CUSTOM_FIELDS_FOLLOW_UP_LIST,
)
from helpdesk.signals import new_ticket_done
from helpdesk.user import HelpdeskUser
from helpdesk.validators import validate_file_extension
if helpdesk_settings.HELPDESK_KB_ENABLED:
@@ -143,6 +144,13 @@ class EditTicketForm(CustomFieldMixin, forms.ModelForm):
# Disable and add help_text to the merged_to field on this form
self.fields["merged_to"].disabled = True
# A disabled field's value is ignored on submit, so its queryset only
# decides what is rendered. Left at the default manager it listed the id
# and title of every ticket in the installation, including queues the
# viewer cannot open, so it is narrowed to the value actually in use.
self.fields["merged_to"].queryset = Ticket.objects.filter(
pk=self.instance.merged_to_id
)
self.fields["merged_to"].help_text = _(
"This ticket is merged into the selected ticket."
)
@@ -735,10 +743,26 @@ class TicketResolvesForm(forms.ModelForm):
class MultipleTicketSelectForm(forms.Form):
tickets = forms.ModelMultipleChoiceField(
label=_("Tickets to merge"),
queryset=Ticket.objects.filter(merged_to=None),
queryset=Ticket.objects.none(),
widget=forms.SelectMultiple(attrs={"class": "form-control"}),
)
def __init__(self, *args, user=None, **kwargs):
"""The choices are narrowed to what `user` may open.
merge_tickets() already refuses to merge a ticket from a queue the
caller cannot access, but the picker itself listed the id and title of
every unmerged ticket in the installation. Passing the user keeps the
two consistent: what cannot be merged is not offered either.
"""
super().__init__(*args, **kwargs)
tickets = (
HelpdeskUser(user).accessible_tickets()
if user is not None
else Ticket.objects.none()
)
self.fields["tickets"].queryset = tickets.filter(merged_to=None)
def clean_tickets(self):
tickets = self.cleaned_data.get("tickets")
if len(tickets) < 2:

View File

@@ -10,7 +10,7 @@ from model_utils import Choices
from helpdesk.serializers import DatatablesTicketSerializer
from .models import FollowUp
from .models import FollowUp, Ticket
def query_to_base64(query):
@@ -204,6 +204,12 @@ class __Query__:
# Now remove the matched null keys
for null_key in matched_null_keys:
del null_filters[null_key]
# Applied here as well as in get(), so that a HELPDESK_QUERY_CLASS
# overriding get() to supply a different base queryset keeps the
# boundary. A subclass can override this method too, so this is a
# narrower surface rather than a guarantee. As a Q on the same query,
# rather than a join against a list of queues.
queryset = queryset.filter(self.huser.accessible_tickets_q())
queryset = queryset.filter(
*q_args,
(Q(**value_filters) & Q(**null_filters)) & self.get_search_filter_args(),
@@ -223,9 +229,9 @@ class __Query__:
return queryset.distinct()
def get(self):
# Prefilter the allowed tickets
tickets = self.huser.get_tickets_in_queues().select_related()
return self.__run__(tickets)
# __run__() applies the boundary itself; this is the select_related()
# that the list views want, not the authorization step.
return self.__run__(Ticket.objects.select_related())
def get_datatables_context(self, *, column_lookup=None, **kwargs):
"""

View File

@@ -200,12 +200,12 @@
<i class="fas fa-link"></i>
</a>
<ul class="list-unstyled">
{% for resolves in ticket.depends_on.all %}
{% for dep in resolves %}
<li>
<a href="{{ resolves.ticket.get_absolute_url }}">#{{ resolves.ticket.id }} {{ resolves.ticket.title|truncatechars:38 }}</a>
<a href="{{ dep.ticket.get_absolute_url }}">#{{ dep.ticket.id }} {{ dep.ticket.title|truncatechars:38 }}</a>
<a class="link-secondary ms-1"
data-bs-toggle="tooltip"
href="{% url 'helpdesk:ticket_resolves_del' resolves.ticket.id resolves.id %}"
href="{% url 'helpdesk:ticket_resolves_del' dep.ticket.id dep.id %}"
title='{% translate "Drop the dependency on this ticket. A ticket may not be closed until all tickets it depends on are closed or removed." %}'>
<i class="fa-regular fa-trash-can"></i>
</a>

View File

@@ -1,3 +1,5 @@
from django.db.models import Q
from helpdesk import settings as helpdesk_settings
from helpdesk.models import Queue, Ticket
@@ -49,8 +51,47 @@ class HelpdeskUser:
kbitems.append(item)
return kbitems
def get_tickets_in_queues(self):
return Ticket.objects.filter(queue__in=self.get_queues())
def accessible_queues(self):
"""Queues this user may actually open a ticket in.
Narrower than get_queues(), which also returns every queue accepting
public submissions so that they appear in the queue pickers.
"""
return [q for q in self.get_queues() if self.can_access_queue(q)]
def accessible_tickets_q(self):
"""can_access_ticket() as a Q, to apply to any Ticket queryset.
Expressed as a Q rather than a queryset so a caller that already has one
can narrow it in the same query instead of joining against a subquery.
Matching can_access_ticket() means a granted queue or a ticket assigned
to this user, whatever queue that ticket lives in.
Queue permissions are read with get_all_permissions() rather than
get_user_permissions(): the latter omits permissions held through a
group, which is how installations of any size grant queue access.
"""
if self.has_full_access():
return Q()
if not self.user.is_authenticated:
return Q(pk__in=[])
reachable = Q(assigned_to=self.user)
if helpdesk_settings.HELPDESK_ENABLE_PER_QUEUE_STAFF_PERMISSION:
# Guarded on the setting for the same reason can_access_queue() is:
# with per-queue mode off, a queue permission grants nothing, and
# the two must not disagree about that.
reachable |= Q(queue__permission_name__in=self.user.get_all_permissions())
return reachable
def accessible_tickets(self):
"""The queryset form of can_access_ticket(), for views and viewsets that
resolve a ticket from a caller-supplied id.
Single place where the rule lives: every path that serves or mutates a
ticket should start from this rather than from Ticket.objects, so that
forgetting the check is not the default.
"""
return Ticket.objects.filter(self.accessible_tickets_q())
def has_full_access(self):
if self.user.is_superuser:

View File

@@ -20,29 +20,6 @@ from helpdesk.serializers import (
from helpdesk.user import HelpdeskUser
def accessible_tickets(user):
"""The tickets `user` is allowed to reach through the API.
This is the queryset form of the check the staff views actually apply, which
is `can_access_queue()` on the ticket's queue.
Two helpers look like they belong here and do not. `get_queues()` is wider:
it also returns every queue accepting public submissions, so it covers
tickets the UI refuses to open. `can_access_ticket()` looks wider too, since
it grants a ticket assigned to the user whatever its queue, but that branch
never runs in the UI: `ticket_perm_check()` tests the queue first and denies
before reaching it. Granting it here would leave the API more permissive than
the interface, which is the shape of the problem this is fixing.
With HELPDESK_ENABLE_PER_QUEUE_STAFF_PERMISSION disabled, or for a
superuser, `has_full_access()` is true, every queue passes and this filter is
a no-op.
"""
huser = HelpdeskUser(user)
queues = [q for q in huser.get_queues() if huser.can_access_queue(q)]
return Ticket.objects.filter(queue__in=queues)
def restrict_relation(serializer, field_name, queryset):
"""Narrow a writable relation so it cannot reference an unreachable object.
@@ -109,7 +86,7 @@ class TicketViewSet(viewsets.ModelViewSet):
permission_classes: ClassVar[list] = [IsAdminUser]
def get_queryset(self):
tickets = accessible_tickets(self.request.user)
tickets = HelpdeskUser(self.request.user).accessible_tickets()
# filter by status
status = self.request.query_params.get("status", None)
@@ -145,7 +122,9 @@ class TicketViewSet(viewsets.ModelViewSet):
# tickets to a foreign one, which both confirms that ticket exists and
# writes a reference into it.
restrict_relation(
serializer, "merged_to", accessible_tickets(self.request.user)
serializer,
"merged_to",
HelpdeskUser(self.request.user).accessible_tickets(),
)
return serializer
@@ -157,11 +136,15 @@ class FollowUpViewSet(viewsets.ModelViewSet):
permission_classes: ClassVar[list] = [IsAdminUser]
def get_queryset(self):
return FollowUp.objects.filter(ticket__in=accessible_tickets(self.request.user))
return FollowUp.objects.filter(
ticket__in=HelpdeskUser(self.request.user).accessible_tickets()
)
def get_serializer(self, *args, **kwargs):
serializer = super().get_serializer(*args, **kwargs)
restrict_relation(serializer, "ticket", accessible_tickets(self.request.user))
restrict_relation(
serializer, "ticket", HelpdeskUser(self.request.user).accessible_tickets()
)
return serializer
def perform_create(self, serializer):
@@ -175,7 +158,9 @@ class FollowUpAttachmentViewSet(viewsets.ModelViewSet):
permission_classes: ClassVar[list] = [IsAdminUser]
def accessible_followups(self):
return FollowUp.objects.filter(ticket__in=accessible_tickets(self.request.user))
return FollowUp.objects.filter(
ticket__in=HelpdeskUser(self.request.user).accessible_tickets()
)
def get_queryset(self):
return FollowUpAttachment.objects.filter(

View File

@@ -32,12 +32,19 @@ class OpenTicketsByUser(Feed):
def get_object(self, request, user_name, queue_slug=None):
user = get_object_or_404(User, username=user_name)
huser = HelpdeskUser(request.user)
if queue_slug:
queue = get_object_or_404(Queue, slug=queue_slug)
if not huser.can_access_queue(queue):
raise PermissionDenied()
else:
queue = None
return {"user": user, "queue": queue}
return {
"user": user,
"queue": queue,
"accessible": huser.accessible_tickets(),
}
def title(self, obj):
if obj["queue"]:
@@ -77,16 +84,13 @@ class OpenTicketsByUser(Feed):
)
def items(self, obj):
# Scoped to the caller, not to obj["user"]: the feed can be requested
# for any username, so filtering only by assignee served another
# person's tickets from every queue.
tickets = obj["accessible"].filter(assigned_to=obj["user"])
if obj["queue"]:
return (
Ticket.objects.filter(assigned_to=obj["user"])
.filter(queue=obj["queue"])
.filter(Q_OPEN_STATUSES)
)
else:
return Ticket.objects.filter(assigned_to=obj["user"]).filter(
Q_OPEN_STATUSES
)
tickets = tickets.filter(queue=obj["queue"])
return tickets.filter(Q_OPEN_STATUSES)
def item_pubdate(self, item):
return item.created
@@ -106,8 +110,11 @@ class UnassignedTickets(Feed):
description = _("Unassigned Open and Reopened tickets")
link = "" # '%s?assigned_to=' % reverse('helpdesk:list')
def get_object(self, request):
return HelpdeskUser(request.user).accessible_tickets()
def items(self, obj):
return Ticket.objects.filter(assigned_to__isnull=True).filter(Q_OPEN_STATUSES)
return obj.filter(assigned_to__isnull=True).filter(Q_OPEN_STATUSES)
def item_pubdate(self, item):
return item.created
@@ -129,8 +136,14 @@ class RecentFollowUps(Feed):
)
link = "/tickets/" # reverse('helpdesk:list')
def items(self):
return FollowUp.objects.order_by("-date")[:20]
def get_object(self, request):
return HelpdeskUser(request.user).accessible_tickets()
def items(self, obj):
# Narrowed before the slice, not after, or the twenty most recent
# follow-ups would be selected across every queue and then filtered
# down to whatever happened to be reachable.
return FollowUp.objects.filter(ticket__in=obj).order_by("-date")[:20]
class OpenTicketsByQueue(Feed):

View File

@@ -347,10 +347,11 @@ dashboard = staff_member_required(dashboard)
def ticket_perm_check(request, ticket):
huser = HelpdeskUser(request.user)
if not huser.can_access_queue(ticket.queue):
raise PermissionDenied()
if not huser.can_access_ticket(ticket):
# can_access_ticket() only. Testing can_access_queue() first denied before
# the assignment branch could ever apply, which made a ticket assigned to
# the user unreachable when it lived outside their queues, and left that
# branch of can_access_ticket() dead.
if not HelpdeskUser(request.user).can_access_ticket(ticket):
raise PermissionDenied()
@@ -379,9 +380,12 @@ delete_ticket = staff_member_required(delete_ticket)
@helpdesk_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)
ticket = get_object_or_404(Ticket, id=ticket_id)
ticket_perm_check(request, ticket)
# Bound to the ticket the caller was just authorized for. Resolving the two
# ids independently only proved access to the one in the URL, which let a
# permitted ticket id be paired with a follow-up from any other queue.
followup = get_object_or_404(FollowUp, id=followup_id, ticket=ticket)
if request.method == "GET":
form = EditFollowUpForm(
@@ -399,17 +403,20 @@ def followup_edit(request, ticket_id, followup_id):
if ticket.status not in Ticket.OPEN_STATUSES:
# If current ticket is closed, add it to the queryset
form.fields["ticket"].queryset = (
Ticket.objects.filter(
Q(id=ticket.id) | Q(status__in=Ticket.OPEN_STATUSES)
)
HelpdeskUser(request.user)
.accessible_tickets()
.filter(Q(id=ticket.id) | Q(status__in=Ticket.OPEN_STATUSES))
.distinct()
.order_by("-id")
)
else:
# If ticket is open, just show open tickets
form.fields["ticket"].queryset = Ticket.objects.filter(
status__in=Ticket.OPEN_STATUSES
).order_by("-id")
form.fields["ticket"].queryset = (
HelpdeskUser(request.user)
.accessible_tickets()
.filter(status__in=Ticket.OPEN_STATUSES)
.order_by("-id")
)
ticketcc_string = return_ticketccstring_and_show_subscribe(
request.user, ticket
@@ -430,10 +437,12 @@ def followup_edit(request, ticket_id, followup_id):
# Needed to allow editing of closed tickets followups
original_ticket = get_object_or_404(Ticket, id=followup.ticket.id)
if original_ticket.status not in Ticket.OPEN_STATUSES:
form.fields["ticket"].queryset = Ticket.objects.filter(
Q(id=original_ticket.id) | Q(status__in=Ticket.OPEN_STATUSES)
).distinct()
form.fields["ticket"].queryset = (
HelpdeskUser(request.user)
.accessible_tickets()
.filter(Q(id=original_ticket.id) | Q(status__in=Ticket.OPEN_STATUSES))
.distinct()
)
if form.is_valid():
# Edit in place: a copy would lose the message ID and the
@@ -447,6 +456,25 @@ def followup_edit(request, ticket_id, followup_id):
followup.save()
return HttpResponseRedirect(reverse("helpdesk:view", args=[ticket.id]))
# An invalid form used to fall off the end of the view and return None.
# Scoping the ticket queryset makes that reachable: an id outside the
# caller's queues is now a validation error rather than an accepted
# value, and a rejected request deserves the form back with its errors.
ticketcc_string = return_ticketccstring_and_show_subscribe(
request.user, ticket
)[0]
return render(
request,
"helpdesk/followup_edit.html",
{
"followup": followup,
"ticket": ticket,
"form": form,
"ticketcc_string": ticketcc_string,
"ticket_attachments": get_attachments_for_ticket(ticket),
},
)
followup_edit = staff_member_required(followup_edit)
@@ -488,10 +516,13 @@ def view_ticket(request, ticket_id):
try:
ticket_perm_check(request, ticket)
except PermissionDenied:
# Identified by id only. Ticket.__str__ includes the title, so
# interpolating the object echoed the title of a ticket the user was
# just refused, turning the denial itself into a disclosure.
messages.error(
request,
_("You don't have permission to view ticket - %(ticket)s.")
% {"ticket": str(ticket)},
_("You don't have permission to view ticket %(ticket_id)s.")
% {"ticket_id": ticket.id},
)
return HttpResponseRedirect(reverse("helpdesk:list"))
@@ -556,10 +587,22 @@ def view_ticket(request, ticket_id):
return redirect("helpdesk:edit_ticket_checklist", ticket.id, checklist.id)
# Both dependency directions expose the id and title of the other ticket, so
# they are scoped to what this user may open. Without it, opening a ticket of
# your own disclosed the title of any ticket linked to it, in any queue, with
# no id manipulation needed.
reachable = HelpdeskUser(request.user).accessible_tickets()
# List open tickets on top
dependencies = ticket.ticketdependency.annotate(
rank=Case(When(depends_on__status__in=Ticket.OPEN_STATUSES, then=1), default=2)
).order_by("rank")
dependencies = (
ticket.ticketdependency.filter(depends_on__in=reachable)
.annotate(
rank=Case(
When(depends_on__status__in=Ticket.OPEN_STATUSES, then=1), default=2
)
)
.order_by("rank")
)
resolves = ticket.depends_on.filter(ticket__in=reachable)
# add custom fields to further details panel
customfields_form = EditTicketCustomFieldForm(None, instance=ticket)
@@ -571,6 +614,7 @@ def view_ticket(request, ticket_id):
"ticket": ticket,
"followups": get_followups_for_ticket(ticket),
"dependencies": dependencies,
"resolves": resolves,
"ticket_attachments": get_attachments_for_ticket(ticket),
"submitter_userprofile_url": submitter_userprofile_url,
"form": form,
@@ -676,7 +720,11 @@ def get_ticket_from_request_with_authorisation(
except (Ticket.DoesNotExist, ValueError):
raise PermissionDenied()
return get_object_or_404(Ticket, id=ticket_id)
# Staff branch: scoped to the queues this user may open, so the POST side of
# an update cannot reach further than its GET side.
return get_object_or_404(
HelpdeskUser(request.user).accessible_tickets(), id=ticket_id
)
def get_due_date_from_form_or_ticket(form, ticket: Ticket) -> datetime.date | None:
@@ -1067,7 +1115,9 @@ def merge_tickets(request):
data to keep per attributes as well as custom fields.
Follow-ups and ticketCC will be moved to the main ticket and other tickets won't be able to receive new answers.
"""
ticket_select_form = MultipleTicketSelectForm(request.GET or None)
ticket_select_form = MultipleTicketSelectForm(
request.GET or None, user=request.user
)
tickets = custom_fields = None
if ticket_select_form.is_valid():
tickets = ticket_select_form.cleaned_data.get("tickets")
@@ -1136,7 +1186,10 @@ def check_redirect_on_user_query(request, huser):
if filter_:
try:
ticket = huser.get_tickets_in_queues().get(**filter_)
# accessible_tickets(), not get_tickets_in_queues(): searching
# a ticket number redirected to a ticket the caller would then
# be refused, which confirms the id exists.
ticket = huser.accessible_tickets().get(**filter_)
return HttpResponseRedirect(ticket.staff_url)
except Ticket.DoesNotExist:
# Go on to standard keyword searching
@@ -1464,8 +1517,14 @@ class UpdateTicketView(
return kwargs
def get_object(self, queryset=None):
# Starts from the authorized queryset rather than the default manager,
# so a foreign-queue id is a 404 here as it is in the sibling routes.
# Those call ticket_perm_check() after resolving; this class resolves
# through the queryset instead, which cannot be forgotten separately.
ticket_id = self.kwargs["ticket_id"]
return Ticket.objects.get(id=ticket_id)
return get_object_or_404(
HelpdeskUser(self.request.user).accessible_tickets(), id=ticket_id
)
def form_valid(self, form):
ticket_id = self.kwargs["ticket_id"]
@@ -2411,7 +2470,10 @@ def kanban_update_ticket(request, ticket_id):
return JsonResponse({"error": "POST required"}, status=405)
ticket = get_object_or_404(Ticket, id=ticket_id)
huser = HelpdeskUser(request.user)
if ticket.queue not in huser.get_queues():
# can_access_queue(), not get_queues(): the latter also returns queues that
# accept public submissions, which would let a status be written on a ticket
# this user cannot open.
if not huser.can_access_queue(ticket.queue):
return JsonResponse({"error": "Permission denied"}, status=403)
try:
data = json.loads(request.body)

View File

@@ -383,14 +383,27 @@ class PerQueuePermissionSecurityTestCase(TestCase):
self.assertTrue(TicketDependency.objects.filter(id=self.dep_2.id).exists())
def test_merge_tickets_blocked_for_inaccessible_queue(self):
"""user_1 cannot merge tickets when one belongs to queue_2."""
"""user_1 cannot merge tickets when one belongs to queue_2.
Asserts the outcome rather than the status code. The selector's queryset
is now scoped to the queues the caller may open, so a foreign id is
refused by form validation before the view's PermissionDenied branch is
reached. Both deny the merge; checking that it did not happen, and that
no foreign title was rendered, holds whichever layer catches it.
"""
self._login_user_1()
url = (
reverse("helpdesk:merge_tickets")
+ f"?tickets={self.ticket_2.id}&tickets={self.ticket_2b.id}"
)
response = self.client.post(url, {"chosen_ticket": self.ticket_2.id})
self.assertEqual(response.status_code, 403)
self.assertIn(response.status_code, (200, 403))
if response.status_code == 200:
self.assertNotContains(response, self.ticket_2.title)
self.ticket_2.refresh_from_db()
self.ticket_2b.refresh_from_db()
self.assertIsNone(self.ticket_2.merged_to_id)
self.assertIsNone(self.ticket_2b.merged_to_id)
def test_rss_queue_blocked_for_inaccessible_queue(self):
"""user_1 cannot read the RSS feed for queue_2."""
@@ -409,10 +422,13 @@ class PerQueuePermissionSecurityTestCase(TestCase):
stored_messages = list(response.context["messages"])
self.assertEqual(len(stored_messages), 1)
self.assertEqual(stored_messages[0].level, messages.ERROR)
# Identified by id: including the ticket's string representation echoed
# the title of a ticket the user was just refused.
self.assertEqual(
str(stored_messages[0]),
f"You don't have permission to view ticket - {self.ticket_2}.",
f"You don't have permission to view ticket {self.ticket_2.id}.",
)
self.assertNotIn(self.ticket_2.title, str(stored_messages[0]))
def test_view_ticket_allowed_for_accessible_queue(self):
"""The redirect only applies when permission is denied: user_1 can
@@ -599,14 +615,29 @@ class PerQueueApiAuthorizationTestCase(TestCase):
self.assertEqual(response.status_code, 201, response.content)
def test_api_and_ui_agree_on_a_ticket_assigned_outside_the_user_queues(self):
"""`can_access_ticket()` reads as though an assignee may always reach
their ticket, but `ticket_perm_check()` tests the queue first and denies
before that branch runs, so the UI refuses it. The API must refuse it
too: granting it would leave the API more permissive than the interface.
"""A ticket assigned to the user is reachable whatever queue it lives
in, and both surfaces must say so.
`can_access_ticket()` always read that way, but `ticket_perm_check()`
tested the queue first and denied before the assignment branch could
run, so the branch was dead and the UI refused. Both now follow
can_access_ticket(), which is what the interface claimed all along.
"""
self.ticket_b.assigned_to = self.staff
self.ticket_b.save()
self.login_staff()
ui = self.client.get(
reverse("helpdesk:view", kwargs={"ticket_id": self.ticket_b.id})
)
self.assertEqual(ui.status_code, 200)
self.assertEqual(
self.client.get(f"/api/tickets/{self.ticket_b.id}/").status_code, 200
)
def test_an_unassigned_ticket_outside_the_user_queues_is_still_refused(self):
"""The control for the test above: the assignment is what grants it, not
the widening itself."""
self.login_staff()
ui = self.client.get(
reverse("helpdesk:view", kwargs={"ticket_id": self.ticket_b.id}),
follow=True,

View File

@@ -0,0 +1,497 @@
"""
Every staff route that resolves an object from a caller-supplied id must deny a
foreign queue.
Four advisories in five weeks came from the same shape: authorization is opt-in
per view, `ticket_perm_check()` is called by most of them and forgotten by one,
and nothing makes forgetting it fail. Each report found the next view that had
been missed.
So rather than another list of hand-written cases, this walks the URL conf and
requires every route to be classified: either it is exercised here against a
foreign queue, or it is named in NO_TICKET_DATA with a reason. A route in
neither fails, so a new route cannot be added without deciding its boundary.
An earlier version keyed on routes taking a `ticket_id`, which is why the RSS
feeds escaped it: they address tickets by assignee, by queue slug, or by nothing
at all. Classifying every route is what makes the guarantee match its claim.
The assertion is deliberately about content rather than status code. Denial is
expressed as 403 by some views, 404 by others and a redirect with a message by
`view_ticket`, and pinning a number per route would make this brittle without
making it stricter. What must never happen is foreign-queue content reaching the
response body.
"""
import json
from django.contrib.auth import get_user_model
from django.contrib.auth.models import Group, Permission
from django.core.files.uploadedfile import SimpleUploadedFile
from django.test import TestCase
from django.urls import reverse
from helpdesk import settings
from helpdesk.models import (
Checklist,
FollowUp,
FollowUpAttachment,
Queue,
Ticket,
TicketCC,
TicketDependency,
)
from helpdesk.query import query_to_base64
from helpdesk.user import HelpdeskUser
# Appears in the foreign ticket and in every object hanging off it. If any
# response body contains it, something served across the boundary.
MARKER = "SWEEPMARKER_FOREIGN_QUEUE_CONTENT"
# Routes that cannot serve ticket data, with the reason each one is exempt.
# Every route in the URL conf must be either exercised by this sweep or listed
# here; one in neither fails test_every_route_is_classified, so a new route
# cannot be added without someone deciding which it is.
NO_TICKET_DATA = {
"savequery": "stores a search definition, returns no ticket",
"delete_query": "acts on a SavedSearch owned by the caller",
"user_settings": "the caller's own UserSettings",
"email_ignore": "IgnoreEmail rows, not tickets",
"email_ignore_add": "IgnoreEmail rows, not tickets",
"email_ignore_del": "IgnoreEmail rows, not tickets",
"checklist_templates": "ChecklistTemplate rows, not attached to a ticket",
"edit_checklist_template": "ChecklistTemplate rows, not attached to a ticket",
"delete_checklist_template": "ChecklistTemplate rows, not attached to a ticket",
"raw": "serves PreSetReply bodies only; preset scoping is its own question",
"home": "public portal landing page",
"submit": "public submission form",
"submit_iframe": "public submission form",
"success_iframe": "public submission confirmation",
"public_view": "public portal, gated by submitter email or secret key "
"rather than by queue permissions",
"public_change_language": "sets a language cookie",
"kb_index": "knowledge base",
"kb_category": "knowledge base",
"kb_category_iframe": "knowledge base",
"kb_vote": "knowledge base",
"login": "authentication",
"logout": "authentication",
"password_change": "authentication",
"password_change_done": "authentication",
"help_context": "static help page",
"system_settings": "renders configured settings, no ticket data",
}
class ForeignQueueSweepTestCase(TestCase):
def setUp(self):
original = settings.HELPDESK_ENABLE_PER_QUEUE_STAFF_PERMISSION
settings.HELPDESK_ENABLE_PER_QUEUE_STAFF_PERMISSION = True
self.addCleanup(
setattr,
settings,
"HELPDESK_ENABLE_PER_QUEUE_STAFF_PERMISSION",
original,
)
User = get_user_model()
# Neither queue accepts public submissions, so this measures the queue
# permission boundary and not the separate widening that
# allow_public_submission causes in get_queues().
self.mine = Queue.objects.create(title="Mine", slug="mine")
self.theirs = Queue.objects.create(title="Theirs", slug="theirs")
self.staff = User.objects.create(
username="restricted", is_staff=True, email="r@example.com"
)
self.staff.set_password("pass")
self.staff.save()
self.staff.user_permissions.add(
Permission.objects.get(codename=self.mine.permission_name[9:])
)
# Owns the foreign ticket, so the feeds keyed on an assignee rather than
# on a ticket id have something to serve across the boundary.
self.victim = User.objects.create(
username="victim", is_staff=True, email="v@example.com"
)
self.my_ticket = Ticket.objects.create(title="Mine", queue=self.mine)
self.foreign = Ticket.objects.create(
title=MARKER,
queue=self.theirs,
description=MARKER,
assigned_to=self.victim,
)
# Unassigned, so the unassigned-tickets feed has something to serve too.
# The ticket above is assigned to the victim and would never appear in it.
self.foreign_unassigned = Ticket.objects.create(
title=MARKER, queue=self.theirs, description=MARKER
)
self.foreign_followup = FollowUp.objects.create(
ticket=self.foreign, title=MARKER, comment=MARKER, public=False
)
self.foreign_attachment = FollowUpAttachment.objects.create(
followup=self.foreign_followup,
file=SimpleUploadedFile(
f"{MARKER}.txt", MARKER.encode(), content_type="text/plain"
),
filename=f"{MARKER}.txt",
)
self.foreign_cc = TicketCC.objects.create(
ticket=self.foreign, email="cc@example.com"
)
self.foreign_checklist = Checklist.objects.create(
ticket=self.foreign, name=MARKER
)
self.foreign_dependency = TicketDependency.objects.create(
ticket=self.foreign, depends_on=self.my_ticket
)
self.client.login(username="restricted", password="pass")
def foreign_kwargs(self, names):
"""Values naming objects that all belong to the foreign queue.
A route whose parameters are not listed here fails rather than being
skipped: an unrecognised parameter means a new route shape that nobody
has decided the boundary for, which is exactly what this is here to
catch.
"""
known = {
"ticket_id": self.foreign.id,
"followup_id": self.foreign_followup.id,
"attachment_id": self.foreign_attachment.id,
"cc_id": self.foreign_cc.id,
"dependency_id": self.foreign_dependency.id,
"checklist_id": self.foreign_checklist.id,
}
unknown = set(names) - set(known)
if unknown:
self.fail(
f"Route parameters {sorted(unknown)} are not covered by this "
"sweep. Add a foreign-queue value for them so the new route is "
"actually exercised."
)
return {name: known[name] for name in names}
def all_routes(self):
"""Every helpdesk route, read from the URL conf as (name, params).
Reads helpdesk.urls directly rather than the resolver's reverse_dict,
which does not expose namespaced routes from the root resolver.
"""
from helpdesk import urls as helpdesk_urls
found = []
for entry in helpdesk_urls.urlpatterns:
name = getattr(entry, "name", None)
pattern = getattr(entry, "pattern", None)
if not name or pattern is None:
continue # an include(), not a route
found.append((name, tuple(pattern.regex.groupindex)))
return sorted(set(found))
def ticket_routes(self):
"""The subset taking a ticket_id, for the two id-pairing sweeps."""
return [
(f"helpdesk:{name}", params)
for name, params in self.all_routes()
if "ticket_id" in params
]
def other_routes(self):
"""Routes that serve ticket data without taking a ticket_id.
This is the family the earlier version of this sweep missed: they
address tickets by assignee, by queue slug, by an encoded query, or by
nothing at all, so keying on `ticket_id` never reached them.
"""
return [
(name, params)
for name, params in self.all_routes()
if "ticket_id" not in params and name not in NO_TICKET_DATA
]
def other_kwargs(self, name, params):
"""Parameters for a non-ticket_id route, chosen so that an unscoped view
would serve the foreign ticket."""
known = {
"user_name": self.victim.username,
"queue_slug": self.theirs.slug,
"report": "queuestatus",
"query": query_to_base64(
{"filtering": {}, "sorting": "created", "search_string": ""}
),
}
unknown = set(params) - set(known)
if unknown:
self.fail(
f"Route {name} takes {sorted(unknown)}, which this sweep cannot "
"build. Add a value that would expose the foreign queue, or "
"list the route in NO_TICKET_DATA with a reason."
)
return {p: known[p] for p in params}
def test_every_route_is_classified(self):
"""The guarantee this file actually makes.
A route is either exercised against a foreign queue below or named in
NO_TICKET_DATA. One in neither means nobody decided whether it can serve
a ticket, which is how the RSS feeds went unnoticed through three
advisories: the sweep keyed on `ticket_id` and they take none.
"""
exercised = {
name for name, _ in self.all_routes() if name not in NO_TICKET_DATA
}
self.assertGreater(len(exercised), 20)
for name, params in self.all_routes():
with self.subTest(route=name):
self.assertTrue(
name in NO_TICKET_DATA or name in exercised,
f"Route {name} is classified neither way.",
)
def test_no_route_without_a_ticket_id_serves_foreign_content(self):
"""Dashboards, listings, reports and feeds reach tickets without an id
in the URL, so no id has to be manipulated to test them."""
routes = self.other_routes()
self.assertGreater(len(routes), 10, routes)
for name, params in routes:
with self.subTest(route=name):
url = reverse(
f"helpdesk:{name}", kwargs=self.other_kwargs(name, params)
)
response = self.client.get(url, follow=True)
self.assertNotIn(
MARKER,
response.content.decode(errors="replace"),
f"{name} served content from a queue the user cannot access",
)
def test_the_sweep_actually_found_the_routes(self):
"""Guards against the walk silently matching nothing, which would make
every assertion below vacuous."""
names = [name for name, _ in self.ticket_routes()]
self.assertGreater(len(names), 8, names)
for expected in ("helpdesk:view", "helpdesk:edit", "helpdesk:update"):
self.assertIn(expected, names)
def test_no_route_serves_foreign_queue_content(self):
for name, params in self.ticket_routes():
with self.subTest(route=name):
url = reverse(name, kwargs=self.foreign_kwargs(params))
response = self.client.get(url, follow=True)
body = response.content.decode(errors="replace")
self.assertNotIn(
MARKER,
body,
f"{name} served content from a queue the user cannot access",
)
def test_no_route_mutates_a_foreign_ticket(self):
"""A POST with no useful payload should be rejected on authorization,
before validation has anything to say about the body."""
before = {
"title": self.foreign.title,
"queue": self.foreign.queue_id,
"status": self.foreign.status,
"on_hold": self.foreign.on_hold,
}
for name, params in self.ticket_routes():
with self.subTest(route=name):
url = reverse(name, kwargs=self.foreign_kwargs(params))
self.client.post(url, {}, follow=True)
self.foreign.refresh_from_db()
self.assertEqual(
{
"title": self.foreign.title,
"queue": self.foreign.queue_id,
"status": self.foreign.status,
"on_hold": self.foreign.on_hold,
},
before,
f"{name} modified a ticket in a queue the user cannot access",
)
def test_no_route_accepts_a_foreign_child_under_a_permitted_ticket(self):
"""The mismatched-key shape, which the sweep above cannot see.
Pairing a foreign parent with foreign children only proves that a
missing check is caught. A route that authorizes the ticket in the URL
and then resolves a follow-up, attachment or checklist id independently
needs the opposite pairing: a ticket the caller may reach, and a child
belonging to someone else's queue. That is CWE-639, and it is how the
second half of GHSA-jh3j-fh4x-98pj worked.
"""
mine = {"ticket_id": self.my_ticket.id}
for name, params in self.ticket_routes():
children = [p for p in params if p != "ticket_id"]
if not children:
continue
with self.subTest(route=name):
kwargs = dict(mine, **self.foreign_kwargs(children))
url = reverse(name, kwargs=kwargs)
body = self.client.get(url, follow=True).content.decode(
errors="replace"
)
self.assertNotIn(
MARKER,
body,
f"{name} served a child object from a foreign queue when "
"paired with a ticket the user may access",
)
self.client.post(url, {}, follow=True)
self.foreign_followup.refresh_from_db()
self.assertEqual(
self.foreign_followup.ticket_id,
self.foreign.id,
f"{name} moved a foreign follow-up",
)
def test_permitted_queue_still_works(self):
"""The sweep must not be passing because everything is broken."""
response = self.client.get(
reverse("helpdesk:view", kwargs={"ticket_id": self.my_ticket.id})
)
self.assertEqual(response.status_code, 200)
response = self.client.get(
reverse("helpdesk:update", kwargs={"ticket_id": self.my_ticket.id})
)
self.assertEqual(response.status_code, 200)
def test_kanban_update_checks_the_authorization_helper(self):
"""kanban_update_ticket filtered on get_queues(), which also returns
queues accepting public submissions, so a status could be written on a
ticket the user cannot open."""
public_queue = Queue.objects.create(
title="Public", slug="pub", allow_public_submission=True
)
ticket = Ticket.objects.create(title="Public one", queue=public_queue)
response = self.client.post(
reverse("helpdesk:kanban_update", kwargs={"ticket_id": ticket.id}),
json.dumps({"status": Ticket.CLOSED_STATUS}),
content_type="application/json",
)
self.assertEqual(response.status_code, 403)
ticket.refresh_from_db()
self.assertNotEqual(ticket.status, Ticket.CLOSED_STATUS)
def test_forged_relation_is_rejected_with_a_response(self):
"""Scoping the follow-up form's ticket queryset turns a foreign id into
a validation error, and the view has to answer that with the form and
its errors rather than falling off the end and returning None."""
my_followup = FollowUp.objects.create(
ticket=self.my_ticket, title="Mine", comment="Mine"
)
response = self.client.post(
reverse(
"helpdesk:followup_edit",
kwargs={
"ticket_id": self.my_ticket.id,
"followup_id": my_followup.id,
},
),
{
"title": "Moved",
"ticket": self.foreign.id,
"comment": "Moved",
"new_status": self.my_ticket.status,
"time_spent": "",
},
)
self.assertEqual(response.status_code, 200)
self.assertNotContains(response, MARKER)
my_followup.refresh_from_db()
self.assertEqual(my_followup.ticket_id, self.my_ticket.id)
def test_the_query_class_applies_the_boundary_itself(self):
"""A HELPDESK_QUERY_CLASS that overrides get() must not be able to drop
the queue boundary, so __run__() applies it rather than trusting the
queryset it is handed."""
from helpdesk.query import __Query__
from helpdesk.user import HelpdeskUser
query = __Query__(
HelpdeskUser(self.staff),
query_params={"filtering": {}, "sorting": "created", "search_string": ""},
)
results = query.__run__(Ticket.objects.all())
self.assertNotIn(self.foreign, results)
self.assertIn(self.my_ticket, results)
class AccessibleTicketsParityTestCase(TestCase):
"""The queryset and the predicate must agree, ticket by ticket.
accessible_tickets() exists so that one rule decides what a user may reach.
That only holds while it returns exactly what can_access_ticket() answers
for the same ticket, and the two are written differently enough that they
can drift: one reads has_perm() per queue, the other compares a stored
permission name against the user's permission set.
"""
def build(self):
User = get_user_model()
granted = Queue.objects.create(title="Granted", slug="granted")
other = Queue.objects.create(title="Other", slug="other")
public = Queue.objects.create(
title="Public", slug="public", allow_public_submission=True
)
users = {
"staff": User.objects.create(username="s", is_staff=True),
"non-staff": User.objects.create(username="n", is_staff=False),
"superuser": User.objects.create(
username="r", is_staff=True, is_superuser=True
),
"grouped": User.objects.create(username="g", is_staff=True),
}
permission = Permission.objects.get(codename=granted.permission_name[9:])
for label, user in users.items():
if label == "grouped":
# Holds the queue permission through a group rather than
# directly, which get_user_permissions() would not report.
group = Group.objects.create(name="agents")
group.permissions.add(permission)
user.groups.add(group)
else:
user.user_permissions.add(permission)
tickets = [
Ticket.objects.create(title="in a granted queue", queue=granted),
Ticket.objects.create(title="in another queue", queue=other),
Ticket.objects.create(title="in a public queue", queue=public),
Ticket.objects.create(
title="assigned, in another queue",
queue=other,
assigned_to=users["staff"],
),
]
return users, tickets
def test_the_queryset_matches_the_predicate(self):
users, tickets = self.build()
original = settings.HELPDESK_ENABLE_PER_QUEUE_STAFF_PERMISSION
self.addCleanup(
setattr,
settings,
"HELPDESK_ENABLE_PER_QUEUE_STAFF_PERMISSION",
original,
)
User = get_user_model()
for per_queue in (True, False):
settings.HELPDESK_ENABLE_PER_QUEUE_STAFF_PERMISSION = per_queue
for label, known in users.items():
# Re-read so the permission cache from a previous pass is gone.
huser = HelpdeskUser(User.objects.get(username=known.username))
reachable = set(huser.accessible_tickets().values_list("id", flat=True))
for ticket in tickets:
with self.subTest(
per_queue=per_queue, user=label, ticket=ticket.title
):
self.assertEqual(
huser.can_access_ticket(ticket),
ticket.id in reachable,
f"accessible_tickets() and can_access_ticket() "
f"disagree about '{ticket.title}' for the {label} "
f"account with per-queue mode {per_queue}",
)

View File

@@ -23,41 +23,41 @@ __metadata:
languageName: node
linkType: hard
"datatables.net-bs5@npm:^3.0.3":
version: 3.0.3
resolution: "datatables.net-bs5@npm:3.0.3"
"datatables.net-bs5@npm:^3.0.4":
version: 3.0.4
resolution: "datatables.net-bs5@npm:3.0.4"
dependencies:
datatables.net: "npm:3.0.3"
checksum: 10c0/1a6a43a75e7633ed56f0e1596f51658acc21f578f539446eaf947fa8c0eb285d59299d22a9e66c71060b73d22c5382a3fb342ddcddb18765e75caff2e85a8565
datatables.net: "npm:3.0.4"
checksum: 10c0/b532214ee52f367f7448de60e5fdfc20d1df9dd1c9f3a9d4381294f90cea29e8388168ef5fc607af2a62783643dfbd07a937398aaad1c1ff646eac47852b2aa1
languageName: node
linkType: hard
"datatables.net-buttons-bs5@npm:^4.0.2":
version: 4.0.2
resolution: "datatables.net-buttons-bs5@npm:4.0.2"
"datatables.net-buttons-bs5@npm:^4.0.3":
version: 4.0.3
resolution: "datatables.net-buttons-bs5@npm:4.0.3"
dependencies:
datatables.net-bs5: "npm:^3"
datatables.net-buttons: "npm:4.0.2"
checksum: 10c0/9ab1a6ffa59f92eb7ce6d0a864b0c4ae277d6bd300e9987b582bbf3d5ee2d408e66ce193e27669a54784fd377036c5a2befd94588c40f15436d154877d8059e6
datatables.net-buttons: "npm:4.0.3"
checksum: 10c0/00e55dd5282966622688b515b797e6777186d8287c233b98166ed7c464ec167540f0fb36e2286d51719cf93464f669a6b52d1b4254b2c729cd0222ef4a1d4b01
languageName: node
linkType: hard
"datatables.net-buttons-dt@npm:^4.0.1":
version: 4.0.2
resolution: "datatables.net-buttons-dt@npm:4.0.2"
"datatables.net-buttons-dt@npm:^4.0.3":
version: 4.0.3
resolution: "datatables.net-buttons-dt@npm:4.0.3"
dependencies:
datatables.net-buttons: "npm:4.0.2"
datatables.net-buttons: "npm:4.0.3"
datatables.net-dt: "npm:^3"
checksum: 10c0/e38f0aeb3e582a8f477b9069796056edcfeaf5418b8a27582e8075748e4f1bf5093b201754d0d100638f2c69abdc9554076da391c9a6cdff2f5c73bfc4277518
checksum: 10c0/01aa0c4bbebee0e15a81a273dce658e089702df0e976bf546351d49c334727d5e7e0274ed57615c7d19eab51303f2b7dd1a3422b0d233cba11741535fc905b8c
languageName: node
linkType: hard
"datatables.net-buttons@npm:4.0.2, datatables.net-buttons@npm:^4.0.1":
version: 4.0.2
resolution: "datatables.net-buttons@npm:4.0.2"
"datatables.net-buttons@npm:4.0.3, datatables.net-buttons@npm:^4.0.3":
version: 4.0.3
resolution: "datatables.net-buttons@npm:4.0.3"
dependencies:
datatables.net: "npm:^3"
checksum: 10c0/c52ac78df408efb5b225e0aa3670b5787d86c7bd7224d165de04715e6a600124f8c19db6f950ef156a14049e522ecb679851bf20bab987457660976ebf73b47d
checksum: 10c0/966bc7d89a76308e547dc1de1a5f3a6934baaee95b69c6121c2558c6bae9e50b62d0d1c6e42ff688e9a8b9e644c186359c7d15ccd218fa64483740d1cd91a5ec
languageName: node
linkType: hard
@@ -77,10 +77,10 @@ __metadata:
languageName: node
linkType: hard
"datatables.net@npm:3.0.3, datatables.net@npm:^3.0.3":
version: 3.0.3
resolution: "datatables.net@npm:3.0.3"
checksum: 10c0/77aa6299cdfad54d657459af05f983262856b140996e5e265a1aed2ce439336938480c5840fada9d90d91421f678becc42e1598648f1ddf3f57de067ce0f1072
"datatables.net@npm:3.0.4, datatables.net@npm:^3.0.4":
version: 3.0.4
resolution: "datatables.net@npm:3.0.4"
checksum: 10c0/2e54778f25f4fa68f8d0fdfa387a0761788b458bee8d6e4f9291a58760531a375791f5795e99d587613ca7be61f11e2d266055547dc08f26e60e6445161bc3ec
languageName: node
linkType: hard
@@ -89,11 +89,11 @@ __metadata:
resolution: "django-helpdesk@workspace:."
dependencies:
bootstrap: "npm:5.3.8"
datatables.net: "npm:^3.0.3"
datatables.net-bs5: "npm:^3.0.3"
datatables.net-buttons: "npm:^4.0.1"
datatables.net-buttons-bs5: "npm:^4.0.2"
datatables.net-buttons-dt: "npm:^4.0.1"
datatables.net: "npm:^3.0.4"
datatables.net-bs5: "npm:^3.0.4"
datatables.net-buttons: "npm:^4.0.3"
datatables.net-buttons-bs5: "npm:^4.0.3"
datatables.net-buttons-dt: "npm:^4.0.3"
fontawesome-free: "npm:@fortawesome/fontawesome-free@^6.7.2"
jquery: "npm:4.0.0"
jquery-ui: "npm:1.14.2"