diff --git a/backend/accounts/access.py b/backend/accounts/access.py index fc4cfd97..a07e5c28 100644 --- a/backend/accounts/access.py +++ b/backend/accounts/access.py @@ -5,8 +5,10 @@ download view asks the same question rather than carrying a second copy of the answer. The detail view still calls it. """ +from django.db.models import Q from rest_framework.exceptions import PermissionDenied +from accounts.models import Account from common.permissions import is_org_admin _DENIED = "You do not have Permission to perform this action" @@ -33,6 +35,18 @@ def has_account_access(profile, user, account): return profile.id in {assignee.id for assignee in account.assigned_to.all()} +def visible_accounts_qs(profile, user): + """Accounts ``profile`` may open, the queryset form of `has_account_access`. + + Scoped to ``profile.org`` here, unlike the predicate, whose callers fetch + the account with that filter first. + """ + qs = Account.objects.filter(org=profile.org) + if is_org_admin(profile) or user.is_superuser: + return qs + return qs.filter(Q(created_by=profile.user) | Q(assigned_to=profile)).distinct() + + def assert_account_access(profile, user, account): """Raise 403 unless ``profile`` may open ``account``.""" if not has_account_access(profile, user, account): diff --git a/backend/accounts/tests/test_visible_accounts_qs.py b/backend/accounts/tests/test_visible_accounts_qs.py new file mode 100644 index 00000000..1156ba08 --- /dev/null +++ b/backend/accounts/tests/test_visible_accounts_qs.py @@ -0,0 +1,91 @@ +"""`visible_accounts_qs` is `has_account_access` as a queryset. + +The ticket form's account picker, the ticket CSV import and the ticket API's +`validate_account` all ask "which accounts may this member open". The first +two ask through the queryset, the last through the predicate, so the two must +give the same answer for every kind of caller or a picker offers an account +the save path refuses (or the reverse). +""" + +import pytest + +from accounts.access import has_account_access, visible_accounts_qs +from accounts.models import Account +from common.models import Profile +from conftest import rls_org + + +def _account(org, name, created_by=None): + account = Account.objects.create(name=name, org=org) + # `update` rather than `create(created_by=...)`, so a request user left in + # the thread local cannot restamp it. + Account.objects.filter(pk=account.pk).update(created_by=created_by) + account.refresh_from_db() + return account + + +@pytest.fixture +def accounts(org_a, admin_user, regular_user, user_profile): + created = _account(org_a, "Created by member", created_by=regular_user) + assigned = _account(org_a, "Assigned to member", created_by=admin_user) + assigned.assigned_to.add(user_profile) + hidden = _account(org_a, "Hidden from member", created_by=admin_user) + ownerless = _account(org_a, "Nobody's") + return [created, assigned, hidden, ownerless] + + +@pytest.fixture +def superuser(regular_user): + regular_user.is_superuser = True + regular_user.save(update_fields=["is_superuser"]) + return regular_user + + +def _by_predicate(profile, user, accounts): + return {a.id for a in accounts if has_account_access(profile, user, a)} + + +def _by_queryset(profile, user): + return set(visible_accounts_qs(profile, user).values_list("id", flat=True)) + + +@pytest.mark.django_db +class TestVisibleAccountsAgreesWithThePredicate: + def test_plain_member(self, accounts, user_profile, regular_user): + expected = _by_predicate(user_profile, regular_user, accounts) + assert _by_queryset(user_profile, regular_user) == expected + # Both halves of the rule fire, and both refusals do too. + assert expected == {accounts[0].id, accounts[1].id} + + def test_admin(self, accounts, admin_profile, admin_user): + expected = _by_predicate(admin_profile, admin_user, accounts) + assert _by_queryset(admin_profile, admin_user) == expected + assert expected == {a.id for a in accounts} + + def test_superuser_with_a_plain_profile(self, accounts, user_profile, superuser): + assert user_profile.role != "ADMIN" + expected = _by_predicate(user_profile, superuser, accounts) + assert _by_queryset(user_profile, superuser) == expected + assert expected == {a.id for a in accounts} + + def test_member_with_nothing(self, accounts, org_a, user_b): + # A second org_a member who created nothing and is assigned nothing. + profile = Profile.objects.create( + user=user_b, org=org_a, role="USER", is_active=True + ) + assert _by_predicate(profile, user_b, accounts) == set() + assert _by_queryset(profile, user_b) == set() + + def test_two_assignees_do_not_duplicate_the_row( + self, accounts, user_profile, regular_user, admin_profile + ): + accounts[1].assigned_to.add(admin_profile) + rows = list( + visible_accounts_qs(user_profile, regular_user).values_list("id", flat=True) + ) + assert len(rows) == len(set(rows)) + + def test_never_leaves_the_org(self, accounts, org_b, user_b, admin_profile): + with rls_org(org_b): + other = _account(org_b, "Someone else's", created_by=user_b) + assert other.id not in _by_queryset(admin_profile, admin_profile.user) diff --git a/backend/accounts/views.py b/backend/accounts/views.py index 9dc0479d..82576c63 100644 --- a/backend/accounts/views.py +++ b/backend/accounts/views.py @@ -41,7 +41,7 @@ from accounts.serializer import ( from accounts.tasks import send_email, send_email_to_assigned_user from cases.access import visible_cases_qs from cases.models import Case -from cases.serializer import CaseSerializer +from cases.serializer import CaseSerializer, parent_access_context from cases.workflow import TERMINAL_STATUSES from common.custom_fields import validate_payload as validate_custom_fields_payload from common.lookups import get_scoped_or_404 @@ -256,16 +256,12 @@ class AccountsListView(APIView, LimitOffsetPagination): def get_context_data(self, **kwargs): params = self.request.query_params + # The read rule itself, so the list holds exactly what the detail view + # opens for this caller. queryset = annotate_rollups( - self.model.objects.filter(org=self.request.profile.org), + access.visible_accounts_qs(self.request.profile, self.request.user), self.request.profile, ).order_by("-id") - # The list form of `accounts.access.has_account_access`. - if not (is_org_admin(self.request.profile) or self.request.user.is_superuser): - queryset = queryset.filter( - Q(created_by=self.request.profile.user) - | Q(assigned_to=self.request.profile) - ).distinct() if params: if params.get("name"): @@ -348,15 +344,10 @@ class AccountsListView(APIView, LimitOffsetPagination): # full, including leads whose own detail route answers 403 for them. # `/api/cases/` and `/api/opportunities/` already narrow their # equivalents; these two were missed. - member_scope = Q(created_by=self.request.profile.user) | Q( - assigned_to=self.request.profile - ) - narrow_to_member = not is_org_admin(self.request.profile) - - contact_qs = Contact.objects.filter(org=self.request.profile.org) - if narrow_to_member: - contact_qs = contact_qs.filter(member_scope).distinct() - contacts = contact_qs.values("id", "first_name") + # + # Contacts use their own read rule, which is what the save path + # accepts: it adds superusers and account assignment. + contacts = visible_contacts_qs(self.request.profile).values("id", "first_name") context["contacts"] = contacts context["closed_accounts"] = { "offset": offset, @@ -719,6 +710,7 @@ class AccountDetailView(APIView): object_id=self.account.id, org=self.request.profile.org, ).order_by("-id") + account_cases = visible_cases_qs(profile).filter(account=self.account) context.update( { "attachments": AttachmentsSerializer(attachments, many=True).data, @@ -742,7 +734,9 @@ class AccountDetailView(APIView): many=True, ).data, "cases": CaseSerializer( - visible_cases_qs(profile).filter(account=self.account), many=True + account_cases, + many=True, + context=parent_access_context(profile, account_cases), ).data, "teams": TeamsSerializer( Teams.objects.filter(org=self.request.profile.org), many=True diff --git a/backend/cases/access.py b/backend/cases/access.py index 9994040d..2cec1303 100644 --- a/backend/cases/access.py +++ b/backend/cases/access.py @@ -56,6 +56,19 @@ def visible_cases_qs(profile): ).distinct() +def writable_cases_qs(profile): + """Cases ``profile`` may change or reply on. The queryset form of `write`. + + `visible_cases_qs` without the watcher clause, for any list that offers an + action on each row: a watcher can open the ticket but is refused the reply, + so offering them one is a button that answers 403. + """ + qs = Case.objects.filter(org=profile.org) + if is_org_admin(profile): + return qs + return qs.filter(Q(created_by=profile.user) | Q(assigned_to=profile)).distinct() + + def get_case_or_404(profile, pk): """Fetch a case in the requester's org, or raise ``Http404``. diff --git a/backend/cases/analytics_views.py b/backend/cases/analytics_views.py index 787059a4..9ad69009 100644 --- a/backend/cases/analytics_views.py +++ b/backend/cases/analytics_views.py @@ -28,7 +28,7 @@ from cases import analytics from cases.access import is_org_admin from cases.analytics import DEFAULT_SERVICE_DAYS from cases.models import Case -from cases.serializer import CaseSerializer +from cases.serializer import CaseSerializer, parent_access_context from common.permissions import HasOrgContext from common.renderers import CSV_RENDERERS from common.validators import uuid_param @@ -243,7 +243,9 @@ class AnalyticsDrilldownView(_AnalyticsBaseView): cases = Case.objects.filter(org=request.profile.org, id__in=ids).order_by( "-created_at" ) - data = CaseSerializer(cases, many=True).data + data = CaseSerializer( + cases, many=True, context=parent_access_context(request.profile, cases) + ).data return Response({"count": len(data), "results": data}) diff --git a/backend/cases/approvals.py b/backend/cases/approvals.py index 49531616..36da70ba 100644 --- a/backend/cases/approvals.py +++ b/backend/cases/approvals.py @@ -133,6 +133,49 @@ def find_matching_rule(case, trigger_event: str = "pre_close"): return candidates[0] +def close_refusal(case, *, status, closed_on, priority, case_type): + """Why a write may not close ``case``, as ``{field: message}``, or ``None``. + + The close gate, one rule for both API paths that close a single ticket: + the detail PUT/PATCH (`CaseCreateSerializer.validate`) and the board move + (`CaseMoveView.patch`). The move used to set ``status="Closed"`` itself and + skip the gate, so a drag closed a ticket that PATCH refused for want of an + approval. + + ``case`` is the stored record, ``None`` on create; ``status``, + ``closed_on``, ``priority`` and ``case_type`` are the values the write + would leave on it. Only the transition into Closed is judged, so a case + that is already Closed can be edited without re-approving. A rule matches + on priority, case_type and team, so it is evaluated against the incoming + values, or a caller could re-target the case out of the rule and close it + in the same request. ``case`` is restored either way; nothing is saved. + """ + if status != "Closed": + return None + if case is not None and case.status == "Closed": + return None + if not closed_on: + return {"closed_on": "Closed date is required when closing a case"} + if case is None: + return None + + saved = (case.priority, case.case_type) + case.priority, case.case_type = priority, case_type + try: + rule = find_matching_rule(case, trigger_event="pre_close") + finally: + case.priority, case.case_type = saved + if rule is None: + return None + if Approval.objects.filter(case_id=case.pk, rule=rule, state="approved").exists(): + return None + return { + "status": ( + f"An approval is required before this case can be closed (rule: {rule.name})." + ) + } + + class Approval(BaseModel): """A single approval request bound to one Case + ApprovalRule.""" diff --git a/backend/cases/import_views.py b/backend/cases/import_views.py index e42e5db5..b31ec19a 100644 --- a/backend/cases/import_views.py +++ b/backend/cases/import_views.py @@ -2,7 +2,8 @@ Preview reads the uploaded file and returns row-level validation results without touching the DB. Commit re-runs validation and writes inside a -single transaction. Both endpoints are gated to ADMIN or sales-access users +single transaction. Both endpoints are gated by +`common.permissions.can_mass_import` (admins, superusers, sales access) so non-privileged members cannot mass-create cases through this surface. """ @@ -14,22 +15,11 @@ from rest_framework.response import Response from rest_framework.views import APIView from cases.services.csv_import import commit_rows, parse_and_validate -from common.permissions import HasOrgContext +from common.permissions import HasOrgContext, can_mass_import MAX_UPLOAD_BYTES = 5 * 1024 * 1024 # 5 MB; matches the UI hint -def _can_import(profile) -> bool: - """Mass-create requires admin or explicit sales-access permission.""" - if profile is None: - return False - if getattr(profile, "role", None) == "ADMIN": - return True - if getattr(profile, "is_admin", False): - return True - return bool(getattr(profile, "has_sales_access", False)) - - def _read_upload(request): """Return (file_bytes, error_response). One of the two will be None. @@ -83,7 +73,7 @@ class CaseImportPreviewView(APIView): }, ) def post(self, request, *args, **kwargs): - if not _can_import(request.profile): + if not can_mass_import(request.profile): return Response( {"error": True, "message": "Permission denied"}, status=status.HTTP_403_FORBIDDEN, @@ -117,7 +107,7 @@ class CaseImportCommitView(APIView): }, ) def post(self, request, *args, **kwargs): - if not _can_import(request.profile): + if not can_mass_import(request.profile): return Response( {"error": True, "message": "Permission denied"}, status=status.HTTP_403_FORBIDDEN, diff --git a/backend/cases/kanban_views.py b/backend/cases/kanban_views.py index 854993d2..2a30fbbf 100644 --- a/backend/cases/kanban_views.py +++ b/backend/cases/kanban_views.py @@ -6,12 +6,15 @@ Supports both status-based (default) and custom pipeline-based kanban boards. from django.db import transaction from django.db.models import Q from django.shortcuts import get_object_or_404 +from django.utils import timezone from drf_spectacular.utils import OpenApiParameter, extend_schema, inline_serializer from rest_framework import serializers, status from rest_framework.permissions import IsAuthenticated from rest_framework.response import Response from rest_framework.views import APIView +from cases.access import assert_case_write_access, visible_cases_qs +from cases.approvals import close_refusal from cases.models import Case, CasePipeline, CaseStage from cases.serializer import ( CaseKanbanCardSerializer, @@ -97,17 +100,18 @@ class CaseKanbanView(APIView): status="Duplicate" ) - # Apply permission filtering - if not is_org_admin(request.profile) and not request.user.is_superuser: - queryset = queryset.filter( - Q(assigned_to=request.profile) | Q(created_by=request.profile.user) - ) + # The ticket read rule itself, as the list and the detail view apply + # it: watchers included, and no superuser clause, because the detail + # view has none. + queryset = queryset.filter( + pk__in=visible_cases_qs(request.profile).values("pk") + ) # Apply search/filters queryset = self._apply_filters(queryset, request.query_params) if pipeline_id: - return self._get_pipeline_kanban(queryset, pipeline_id, org) + return self._get_pipeline_kanban(queryset, pipeline_id, request) return self._get_status_kanban(queryset) def _apply_filters(self, queryset, params): @@ -184,10 +188,15 @@ class CaseKanbanView(APIView): } ) - def _get_pipeline_kanban(self, queryset, pipeline_id, org): + def _get_pipeline_kanban(self, queryset, pipeline_id, request): """Build kanban data using CasePipeline stages as columns.""" pipeline = get_object_or_404( - CasePipeline, pk=pipeline_id, org=org, is_active=True + CasePipelineListSerializer.with_counts( + CasePipeline.objects.all(), request.profile + ), + pk=pipeline_id, + org=request.profile.org, + is_active=True, ) queryset = queryset.filter(stage__pipeline=pipeline) @@ -239,15 +248,11 @@ class CaseMoveView(APIView): # edit committing between this read and that save would be lost. case = get_object_or_404(Case.objects.select_for_update(), pk=pk, org=org) - # Permission check - if not is_org_admin(request.profile) and not request.user.is_superuser: - if not ( - request.profile.user == case.created_by - or request.profile in case.assigned_to.all() - ): - return Response( - {"error": "Permission denied"}, status=status.HTTP_403_FORBIDDEN - ) + # A move rewrites the ticket's status, stage and order, so it takes the + # ticket's own write rule and nothing wider. This used to add a Django + # superuser clause, which let a superuser who is a plain member of the + # org move any ticket here that `CaseDetailView.patch` refuses them. + assert_case_write_access(request.profile, case) serializer = CaseMoveSerializer(data=request.data) if not serializer.is_valid(): @@ -257,6 +262,8 @@ class CaseMoveView(APIView): ) data = serializer.validated_data + new_stage = case.stage + new_status = case.status # Handle stage change if "stage_id" in data: @@ -274,17 +281,45 @@ class CaseMoveView(APIView): status=status.HTTP_400_BAD_REQUEST, ) - case.stage = stage + new_stage = stage # Auto-update status if stage has maps_to_status if stage.maps_to_status: - case.status = stage.maps_to_status + new_status = stage.maps_to_status else: - case.stage = None + new_stage = None # Handle status change (for status-based kanban) if "status" in data: - case.status = data["status"] + new_status = data["status"] + + # A drag into Closed is a close, so it takes the gate PATCH takes and + # answers a refusal the same way, before anything is written. The board + # has no date field, so the closing date is today, as it is for the + # ticket page's quick status change. Moving back out of Closed needs + # nothing here: the pre_save signal clears `closed_on` and + # `resolved_at` on every save that leaves Closed, this one included. + closed_on = case.closed_on + if new_status == "Closed" and case.status != "Closed": + closed_on = timezone.localdate() + refusal = close_refusal( + case, + status=new_status, + closed_on=closed_on, + priority=case.priority, + case_type=case.case_type, + ) + if refusal: + return Response( + { + "error": True, + "errors": {field: [msg] for field, msg in refusal.items()}, + }, + status=status.HTTP_400_BAD_REQUEST, + ) + case.stage = new_stage + case.status = new_status + case.closed_on = closed_on # Calculate new order # `case.stage`/`case.status` are already the destination by this @@ -332,7 +367,9 @@ class CasePipelineListCreateView(APIView): def get(self, request): """List all pipelines for the organization.""" org = request.profile.org - pipelines = CasePipeline.objects.filter(org=org, is_active=True) + pipelines = CasePipelineListSerializer.with_counts( + CasePipeline.objects.filter(org=org, is_active=True), request.profile + ) serializer = CasePipelineListSerializer(pipelines, many=True) return Response({"pipelines": serializer.data}) @@ -407,7 +444,8 @@ class CasePipelineListCreateView(APIView): # Refresh to include created stages pipeline.refresh_from_db() return Response( - CasePipelineSerializer(pipeline).data, status=status.HTTP_201_CREATED + CasePipelineSerializer(pipeline, context={"request": request}).data, + status=status.HTTP_201_CREATED, ) @@ -422,7 +460,9 @@ class CasePipelineDetailView(APIView): @extend_schema(tags=["Case Pipelines"], responses={200: CasePipelineSerializer}) def get(self, request, pk): pipeline = self.get_object(pk, request.profile.org) - return Response(CasePipelineSerializer(pipeline).data) + return Response( + CasePipelineSerializer(pipeline, context={"request": request}).data + ) @extend_schema( tags=["Case Pipelines"], @@ -445,7 +485,9 @@ class CasePipelineDetailView(APIView): ) pipeline = serializer.save(updated_by=request.user) - return Response(CasePipelineSerializer(pipeline).data) + return Response( + CasePipelineSerializer(pipeline, context={"request": request}).data + ) @extend_schema(tags=["Case Pipelines"], responses={204: None}) def delete(self, request, pk): @@ -497,7 +539,10 @@ class CaseStageCreateView(APIView): ) stage = serializer.save(pipeline=pipeline, org=org, created_by=request.user) - return Response(CaseStageSerializer(stage).data, status=status.HTTP_201_CREATED) + return Response( + CaseStageSerializer(stage, context={"request": request}).data, + status=status.HTTP_201_CREATED, + ) class CaseStageDetailView(APIView): @@ -526,7 +571,7 @@ class CaseStageDetailView(APIView): ) stage = serializer.save(updated_by=request.user) - return Response(CaseStageSerializer(stage).data) + return Response(CaseStageSerializer(stage, context={"request": request}).data) @extend_schema(tags=["Case Stages"], responses={204: None}) def delete(self, request, pk): diff --git a/backend/cases/merge_views.py b/backend/cases/merge_views.py index 5507728e..0227a66a 100644 --- a/backend/cases/merge_views.py +++ b/backend/cases/merge_views.py @@ -20,7 +20,7 @@ from rest_framework.views import APIView from cases.models import Case, EmailMessage from cases.notifications import case_link -from cases.serializer import CaseSerializer +from cases.serializer import CaseSerializer, parent_access_context from cases.signals import _create_activity from common.models import Attachments, Comment from common.permissions import HasOrgContext, is_org_admin @@ -100,7 +100,12 @@ class CaseMergeView(APIView): "error": False, "message": "Tickets already merged.", "already_merged": True, - "target_case": CaseSerializer(target).data, + "target_case": CaseSerializer( + target, + context=parent_access_context( + request.profile, [target] + ), + ).data, "source_case_id": str(source.id), "redirected_url": case_link(target.id), }, @@ -219,7 +224,9 @@ class CaseMergeView(APIView): { "error": False, "message": "Tickets merged", - "target_case": CaseSerializer(target).data, + "target_case": CaseSerializer( + target, context=parent_access_context(request.profile, [target]) + ).data, "source_case_id": str(source.id), "redirected_url": case_link(target.id), }, diff --git a/backend/cases/models.py b/backend/cases/models.py index 6c5f6d00..6bb053b7 100644 --- a/backend/cases/models.py +++ b/backend/cases/models.py @@ -199,36 +199,23 @@ class Case(AssignableMixin, BaseModel): if self.status == "Closed" and not self.closed_on: errors["closed_on"] = _("Closed date is required when closing a case") - # Tier 3 approvals: when transitioning *into* Closed and an active - # rule matches, require a recorded `approved` Approval. Skip the - # check when callers explicitly opt out (e.g. internal auto-flows - # that have already verified the gate or migration data fixtures). - if ( - self.status == "Closed" - and self.org_id - and not getattr(self, "_approval_gate_skip", False) - ): - old_status = None - if self.pk: - try: - old_status = ( - type(self).objects.only("status").get(pk=self.pk).status - ) - except type(self).DoesNotExist: - old_status = None - if old_status != "Closed": - from cases.approvals import Approval, find_matching_rule + # The close gate is `cases.approvals.close_refusal`, the rule the + # detail PUT/PATCH and the board move call too, so the bulk path that + # calls this method cannot drift from them. It judges the transition + # against the stored row: `self` already carries the incoming values. + # An unsaved case has no stored row, so it is judged as a create. + from cases.approvals import close_refusal - rule = find_matching_rule(self, trigger_event="pre_close") - if rule is not None: - has_approved = Approval.objects.filter( - case_id=self.pk, rule=rule, state="approved" - ).exists() - if not has_approved: - errors["status"] = _( - "An approval is required before this case can be " - "closed (rule: %(rule)s)." - ) % {"rule": rule.name} + stored = type(self).objects.filter(pk=self.pk).first() if self.pk else None + refusal = close_refusal( + stored, + status=self.status, + closed_on=self.closed_on, + priority=self.priority, + case_type=self.case_type, + ) + if refusal: + errors.update(refusal) # Parent/child guards. if self.parent_id is not None: diff --git a/backend/cases/parent_views.py b/backend/cases/parent_views.py index 67241825..4b9c9d25 100644 --- a/backend/cases/parent_views.py +++ b/backend/cases/parent_views.py @@ -10,8 +10,19 @@ Three endpoints over and above what `CaseDetailView` already exposes: The link endpoint takes a row lock on both rows so two concurrent agents cannot build a cycle. Cascade close honours ``Org.auto_close_children_on_parent_close`` as the default, and accepts ``cascade`` in the body to override. + +Who may call them follows `cases.access`, the same as the ticket detail view: + +* the tree needs read on the ticket asked about, and a node the caller cannot + read comes back redacted (see `_redacted`); +* linking needs write on the child and read on the new parent, and a parent + the caller cannot read answers exactly like one that does not exist; +* closing needs write on the ticket and on every descendant the cascade would + close, and each of them must pass `close_refusal`. Nothing is written unless + every one of them does. """ +from django.core.exceptions import ValidationError as DjangoValidationError from django.db import transaction from django.shortcuts import get_object_or_404 from django.utils import timezone @@ -20,6 +31,13 @@ from rest_framework.permissions import IsAuthenticated from rest_framework.response import Response from rest_framework.views import APIView +from cases.access import ( + assert_case_read_access, + assert_case_write_access, + has_case_read_access, + has_case_write_access, +) +from cases.approvals import close_refusal from cases.models import Case from cases.parent_guards import check_parent_link from common.models import Activity @@ -41,15 +59,41 @@ def _summary(case): } -def _build_tree(case, depth=0): - """Recurse to ``TREE_MAX_DEPTH``. Returns ``{...summary, children: [...]}``.""" - node = _summary(case) +def _redacted(case): + """Node payload for a ticket the caller may not open. + + Kept in the tree rather than dropped, so a readable grandchild under a + hidden child still sits at its real depth instead of being lost or + re-parented. It carries the id (both clients key rows on it), ``status`` + and ``is_active`` (both clients count the open descendants a cascading + close would take, and the close refuses when one of those is not the + caller's), and nothing else: no name, priority or assignees. + """ + return { + "id": str(case.id), + "name": None, + "restricted": True, + "status": case.status, + "is_active": case.is_active, + } + + +def _build_tree(case, profile, depth=0): + """Recurse to ``TREE_MAX_DEPTH``. Returns ``{...summary, children: [...]}``. + + Each node is the full summary when ``profile`` passes the read rule on it, + and `_redacted` otherwise. + """ + if has_case_read_access(profile, case): + node = _summary(case) + else: + node = _redacted(case) if depth >= TREE_MAX_DEPTH: node["children"] = [] node["truncated"] = True return node children = list(case.children.all().prefetch_related("assigned_to")) - node["children"] = [_build_tree(c, depth + 1) for c in children] + node["children"] = [_build_tree(c, profile, depth + 1) for c in children] return node @@ -73,6 +117,9 @@ class CaseTreeView(APIView): case = get_object_or_404( Case.objects.prefetch_related("assigned_to"), id=pk, org=org ) + # The same answer the ticket detail GET gives: 404 outside the org, + # 403 for a ticket in it that the caller may not open. + assert_case_read_access(request.profile, case) # Return the root of the visible tree: walk up to the highest ancestor # in the same org so a child URL still shows the full incident. root = case @@ -83,7 +130,7 @@ class CaseTreeView(APIView): seen.add(root.parent_id) root = root.parent return Response( - {"root": _build_tree(root), "focus_id": str(case.id)}, + {"root": _build_tree(root, request.profile), "focus_id": str(case.id)}, status=status.HTTP_200_OK, ) @@ -100,6 +147,9 @@ class CaseLinkParentView(APIView): case = Case.objects.select_for_update().filter(id=pk, org=org).first() if case is None: return Response({"detail": "Not found."}, status=status.HTTP_404_NOT_FOUND) + # Moving a ticket in or out of a tree is a change to it, so it takes + # the write rule, as the detail PUT does. Unlinking included. + assert_case_write_access(request.profile, case) former_parent_id = case.parent_id @@ -124,8 +174,17 @@ class CaseLinkParentView(APIView): status=status.HTTP_200_OK, ) - parent = Case.objects.select_for_update().filter(id=parent_id, org=org).first() - if parent is None: + try: + parent = ( + Case.objects.select_for_update().filter(id=parent_id, org=org).first() + ) + except (DjangoValidationError, ValueError, TypeError): + # Not a UUID, so it names no case: the same answer as a missing one. + parent = None + # A parent the caller may not open answers exactly like a missing one. + # Otherwise the link would confirm the ticket exists, return its name, + # and put the caller's ticket under somebody else's. + if parent is None or not has_case_read_access(request.profile, parent): return Response( {"parent_id": "Parent case not found in this organization."}, status=status.HTTP_400_BAD_REQUEST, @@ -169,17 +228,59 @@ class CaseLinkParentView(APIView): ) -def _open_descendants(case, out=None): - """Collect all open (status != Closed) descendants, breadth-first.""" +def _open_descendants(case, out=None, seen=None): + """Collect all open (status != Closed) active descendants, depth-first. + + Recursion goes through closed children too, so an open grandchild under a + closed child is still collected. ``seen`` stops the walk on a stored + cycle, which would otherwise recurse until the worker fell over. Rows are + locked for the length of the caller's transaction. + """ if out is None: out = [] - for child in case.children.all(): + if seen is None: + seen = {case.id} + for child in case.children.select_for_update().prefetch_related("assigned_to"): + if child.id in seen: + continue + seen.add(child.id) if child.status != "Closed" and child.is_active: out.append(child) - _open_descendants(child, out) + _open_descendants(child, out, seen) return out +def _refused(errors): + """The 400 a refused close answers. + + ``errors`` is `close_refusal`'s ``{field: message}`` for the ticket itself, + answered in `CaseMoveView`'s per-field shape, or a sentence about the + cascade, which is about no one field and so rides as a plain string. Both + clients render either shape. + """ + if isinstance(errors, dict): + errors = {field: [msg] for field, msg in errors.items()} + return Response( + {"error": True, "errors": errors}, status=status.HTTP_400_BAD_REQUEST + ) + + +def _close(case, today): + """Close one ticket the way the ordinary close path leaves it. + + ``closed_on`` is today. ``resolved_at`` is stamped by the + ``case_pre_save_stamp_resolved_at`` signal on the transition into Closed, + and is listed in ``update_fields`` so that stamp is saved. + """ + case.status = "Closed" + case.closed_on = today + case.save(update_fields=["status", "closed_on", "resolved_at", "updated_at"]) + + +def _ticket_count(n): + return f"{n} linked ticket{'' if n == 1 else 's'}" + + class CaseCloseWithChildrenView(APIView): permission_classes = (IsAuthenticated, HasOrgContext) @@ -189,6 +290,7 @@ class CaseCloseWithChildrenView(APIView): case = Case.objects.select_for_update().filter(id=pk, org=org).first() if case is None: return Response({"detail": "Not found."}, status=status.HTTP_404_NOT_FOUND) + assert_case_write_access(request.profile, case) if case.status == "Duplicate": return Response( {"detail": "Cannot close a merged case."}, @@ -202,52 +304,72 @@ class CaseCloseWithChildrenView(APIView): cascade = bool(cascade_override) today = timezone.localdate() - # Close the parent first. - if case.status != "Closed": - case.status = "Closed" - case.closed_on = today - if not case.resolved_at: - from django.utils import timezone as _tz - case.resolved_at = _tz.now() - case.save( - update_fields=[ - "status", - "closed_on", - "resolved_at", - "updated_at", - ] + # Every check runs before anything is written, so a refusal leaves the + # whole tree as it was. The ticket itself takes the same close gate + # as a PATCH; `close_refusal` passes it when it is already Closed. + refusal = close_refusal( + case, + status="Closed", + closed_on=today, + priority=case.priority, + case_type=case.case_type, + ) + if refusal: + return _refused(refusal) + + descendants = _open_descendants(case) if cascade else [] + + # A cascade closes each descendant, so each one needs the write rule. + # Counted, never named: some of them the caller cannot even open. + not_writable = [ + d for d in descendants if not has_case_write_access(request.profile, d) + ] + if not_writable: + n = len(not_writable) + return _refused( + f"{_ticket_count(n)} under this one {'is' if n == 1 else 'are'} " + "not yours to close, so nothing was closed. Close this ticket " + "on its own, or ask an admin to close them all.", ) - cascaded = [] - if cascade: - descendants = _open_descendants(case) - for child in descendants: - child.status = "Closed" - child.closed_on = today - if not child.resolved_at: - from django.utils import timezone as _tz + # Named: every descendant left here is one the caller may write, and + # so may open. + gated = [ + d + for d in descendants + if close_refusal( + d, + status="Closed", + closed_on=today, + priority=d.priority, + case_type=d.case_type, + ) + ] + if gated: + names = ", ".join(f'"{d.name}"' for d in gated) + return _refused( + f"{_ticket_count(len(gated))} under this one need an approval " + f"before closing ({names}), so nothing was closed.", + ) - child.resolved_at = _tz.now() - child.save( - update_fields=[ - "status", - "closed_on", - "resolved_at", - "updated_at", - ] - ) - _record( - child, - "PARENT_CLOSED_CASCADE", - { - "parent_id": str(case.id), - "acted_via_cascade": True, - "resolution_comment": resolution_comment[:1000], - }, - actor=request.profile, - ) - cascaded.append(str(child.id)) + if case.status != "Closed": + _close(case, today) + + cascaded = [] + for child in descendants: + _close(child, today) + _record( + child, + "PARENT_CLOSED_CASCADE", + { + "parent_id": str(case.id), + "acted_via_cascade": True, + "resolution_comment": resolution_comment[:1000], + }, + actor=request.profile, + ) + cascaded.append(str(child.id)) return Response( { diff --git a/backend/cases/serializer.py b/backend/cases/serializer.py index 87f62c0e..2eaacabb 100644 --- a/backend/cases/serializer.py +++ b/backend/cases/serializer.py @@ -1,8 +1,10 @@ -from django.db.models import Sum +from django.db.models import Count, Q, Sum from rest_framework import serializers +from accounts.access import has_account_access from accounts.serializer import AccountPickerSerializer -from cases.approvals import Approval, ApprovalRule +from cases.access import has_case_read_access, visible_cases_qs +from cases.approvals import Approval, ApprovalRule, close_refusal from cases.models import ( Case, CasePipeline, @@ -31,6 +33,29 @@ from contacts.serializer import ContactLinkSerializer # - created_on_arrow (frontend computes its own humanized timestamps) +# The refusal `/link/` gives, so the two doors into a tree answer alike. +PARENT_NOT_FOUND = "Parent case not found in this organization." + + +def parent_access_context(profile, cases): + """`CaseSerializer` context: which of these cases' parents ``profile`` may open. + + One query for the whole batch, so a page of tickets costs the same whether + it has one parent or twenty-five. Pass the same list or queryset object the + serializer is then given: iterating a queryset here fills its result cache, + which the serializer reads instead of querying again. + """ + parent_ids = {c.parent_id for c in cases if c.parent_id} + readable = set() + if parent_ids: + readable = set( + visible_cases_qs(profile) + .filter(id__in=parent_ids) + .values_list("id", flat=True) + ) + return {"readable_parent_ids": readable} + + class CaseSerializer(serializers.ModelSerializer): # `{id, name}` only. Seeing this record is not access to its account, and # both clients read only these two; the rest is on `/api/accounts//`. @@ -58,10 +83,33 @@ class CaseSerializer(serializers.ModelSerializer): time_summary = serializers.SerializerMethodField() def get_parent_summary(self, obj): + """The parent's name and status, or only its id when the viewer may + not open it. + + Reading a child is not reading its parent. This used to hand anyone + who could open the child the parent's name and status, so a member + assigned one sub-ticket learned the subject of a ticket the detail view + refuses them. Which parents are readable comes from + `parent_access_context`; a serializer built without it redacts every + parent, since not knowing who is asking is not permission. The redacted + shape matches the one `/tree/` returns for a hidden node. + """ if not obj.parent_id: return None + if obj.parent_id not in self.context.get("readable_parent_ids", ()): + return { + "id": str(obj.parent_id), + "name": None, + "status": None, + "restricted": True, + } p = obj.parent - return {"id": str(p.id), "name": p.name, "status": p.status} + return { + "id": str(p.id), + "name": p.name, + "status": p.status, + "restricted": False, + } def get_child_count(self, obj): # Prefer prefetched count when callers annotated it. @@ -152,10 +200,19 @@ class CaseCreateSerializer(serializers.ModelSerializer): def __init__(self, *args, **kwargs): request_obj = kwargs.pop("request_obj", None) super().__init__(*args, **kwargs) + self.profile = request_obj.profile + self.user = request_obj.user self.org = request_obj.profile.org # Make account read-only on updates (can only be set on creation) if self.instance: self.fields["account"].read_only = True + else: + # An id that matches no account at all reads the same as one the + # caller may not open; see `validate_account`. + self.fields["account"].error_messages["does_not_exist"] = "No such account." + # An id that matches no case reads the same as one in another org or + # one the caller may not open; see `validate_parent`. + self.fields["parent"].error_messages["does_not_exist"] = PARENT_NOT_FOUND def validate_account(self, account): """An account belonging to another org is not a valid link. @@ -168,8 +225,20 @@ class CaseCreateSerializer(serializers.ModelSerializer): was both a cross-tenant write and a cross-tenant read. `validate_parent` below already guarded the other FK on this serializer; this one had been missed. + + Nor is an account in this org that the caller may not open. Linking + one attached the ticket to a company the member cannot see, so the + rule is `has_account_access`, the one the account detail view applies. + An unknown id, another org's account and a hidden one all get the same + message, so a response never confirms that a hidden account exists. + `account` is read-only once the ticket exists (see `__init__`), so this + runs on create only. """ - if account is not None and account.org_id != self.org.id: + if account is None: + return account + if account.org_id != self.org.id or not has_account_access( + self.profile, self.user, account + ): raise serializers.ValidationError("No such account.") return account @@ -189,6 +258,9 @@ class CaseCreateSerializer(serializers.ModelSerializer): case that is already Closed can be edited without re-approving, which is why this compares against the stored status instead of just looking at the incoming one. + + The rule itself is `cases.approvals.close_refusal`, which the board + move (`CaseMoveView.patch`) calls too, so the two cannot drift apart. """ attrs = super().validate(attrs) @@ -205,80 +277,58 @@ class CaseCreateSerializer(serializers.ModelSerializer): if refusal: raise serializers.ValidationError({"parent": refusal}) - if new_status != "Closed": - return attrs - - old_status = getattr(self.instance, "status", None) - if old_status == "Closed": - return attrs - - closed_on = attrs.get("closed_on", getattr(self.instance, "closed_on", None)) - if not closed_on: - raise serializers.ValidationError( - {"closed_on": "Closed date is required when closing a case"} - ) - - if self.instance is not None: - from cases.approvals import Approval, find_matching_rule - - # A rule matches on priority, case_type and team, so evaluate it - # against the values this request is setting, not the stored ones, - # or a caller could re-target the case out of the rule and close it - # in the same PATCH. The instance is restored either way; nothing - # here is saved. - probe = self.instance - saved = (probe.priority, probe.case_type) - probe.priority = attrs.get("priority", probe.priority) - probe.case_type = attrs.get("case_type", probe.case_type) - try: - rule = find_matching_rule(probe, trigger_event="pre_close") - finally: - probe.priority, probe.case_type = saved - if ( - rule is not None - and not Approval.objects.filter( - case_id=self.instance.pk, rule=rule, state="approved" - ).exists() - ): - raise serializers.ValidationError( - { - "status": ( - "An approval is required before this case can be " - f"closed (rule: {rule.name})." - ) - } - ) + refusal = close_refusal( + self.instance, + status=new_status, + closed_on=attrs.get("closed_on", getattr(self.instance, "closed_on", None)), + priority=attrs.get("priority", getattr(self.instance, "priority", None)), + case_type=attrs.get("case_type", getattr(self.instance, "case_type", None)), + ) + if refusal: + raise serializers.ValidationError(refusal) return attrs def validate_name(self, name): - if self.instance: - if ( - Case.objects.filter(name__iexact=name, org=self.org) - .exclude(id=self.instance.id) - .exists() - ): - raise serializers.ValidationError("Case already exists with this name") + """Refuse a name one of the caller's own tickets already carries. - else: - if Case.objects.filter(name__iexact=name, org=self.org).exists(): - raise serializers.ValidationError("Case already exists with this name") + Ticket names carry no DB constraint, so this is a courtesy against + filing the same ticket twice, and it looks only at tickets the caller + may open (`visible_cases_qs`), as the CSV import does. Checked across + the whole org it was an oracle: the refusal for a name held only by a + ticket the caller cannot open confirmed that ticket existed. + """ + clash = visible_cases_qs(self.profile).filter(name__iexact=name) + if self.instance: + clash = clash.exclude(id=self.instance.id) + if clash.exists(): + raise serializers.ValidationError("Case already exists with this name") return name def validate_parent(self, parent): - """Cross-org link prevention only. + """A parent the caller may name: in this org and readable by them. The rest of the linking rules moved to `validate()`, which is the first place with both the incoming `status` and the record being moved. This one stays a field validator because it is the check that decides whether `parent` may be used at all, and because the caller should get it against the `parent` field either way. + + The read rule is the one `/link/` applies: a parent the caller cannot + open answers exactly like one that does not exist. Checked by org + alone, this was the way round the link endpoint: it put the caller's + ticket under somebody else's and, since a missing id is refused and a + hidden one was not, confirmed the hidden ticket existed. Re-sending the + parent a ticket already has is not a new link, so it is let through + even when that parent is hidden from the caller. """ if parent is None: return parent - if parent.org_id != self.org.id: - raise serializers.ValidationError( - "Parent case must belong to the same organization." - ) + if self.instance is not None and parent.id == self.instance.parent_id: + return parent + if parent.org_id != self.org.id or not has_case_read_access( + self.profile, parent + ): + raise serializers.ValidationError(PARENT_NOT_FOUND) return parent class Meta: @@ -332,8 +382,21 @@ class CaseCommentEditSwaggerSerializer(serializers.Serializer): # ============================================ +def _visible_case_count(serializer, **lookup): + """How many cases matching ``lookup`` the requester may open. + + ``case_count`` once counted every case in the org, so a member whose board + showed three cards read a count that included tickets they cannot open. It + follows ``visible_cases_qs`` now, the rule the board and the list use. The + request has to be in the serializer context: a missing one is a KeyError, + never an unscoped count. + """ + profile = serializer.context["request"].profile + return visible_cases_qs(profile).filter(**lookup).count() + + class CaseStageSerializer(serializers.ModelSerializer): - """Serializer for case stages.""" + """Serializer for case stages. Needs ``request`` in its context.""" case_count = serializers.SerializerMethodField() @@ -354,11 +417,12 @@ class CaseStageSerializer(serializers.ModelSerializer): read_only_fields = ("id", "created_at", "updated_at", "org") def get_case_count(self, obj): - return obj.cases.count() + return _visible_case_count(self, stage=obj) class CasePipelineSerializer(serializers.ModelSerializer): - """Serializer for case pipelines with nested stages.""" + """Serializer for case pipelines with nested stages. Needs ``request`` in + its context.""" stages = CaseStageSerializer(many=True, read_only=True) stage_count = serializers.SerializerMethodField() @@ -384,11 +448,17 @@ class CasePipelineSerializer(serializers.ModelSerializer): return obj.stages.count() def get_case_count(self, obj): - return Case.objects.filter(stage__pipeline=obj).count() + return _visible_case_count(self, stage__pipeline=obj) class CasePipelineListSerializer(serializers.ModelSerializer): - """Simplified pipeline serializer for lists.""" + """Simplified pipeline serializer for lists. + + Both counts are read from annotations, so every queryset serialized here + must come through ``with_counts``: one query for the whole list, however + many pipelines. A pipeline without them raises AttributeError, which is + louder than a count that silently ignores who is asking. + """ stage_count = serializers.SerializerMethodField() case_count = serializers.SerializerMethodField() @@ -406,11 +476,23 @@ class CasePipelineListSerializer(serializers.ModelSerializer): "created_at", ] + @staticmethod + def with_counts(pipelines, profile): + # The case join repeats a stage once per case in it, hence distinct on + # the stage count. A case sits in one stage, so it is counted once. + return pipelines.annotate( + stage_count=Count("stages", distinct=True), + case_count=Count( + "stages__cases", + filter=Q(stages__cases__in=visible_cases_qs(profile).values("pk")), + ), + ) + def get_stage_count(self, obj): - return obj.stages.count() + return obj.stage_count def get_case_count(self, obj): - return Case.objects.filter(stage__pipeline=obj).count() + return obj.case_count class CaseKanbanCardSerializer(serializers.ModelSerializer): diff --git a/backend/cases/services/csv_import.py b/backend/cases/services/csv_import.py index 7837b320..0147e9f6 100644 --- a/backend/cases/services/csv_import.py +++ b/backend/cases/services/csv_import.py @@ -5,10 +5,10 @@ beyond read-only lookups; `commit_rows` writes inside a transaction. Both phases re-run validation so the commit endpoint is safe even if called directly. Reference fields (`account_name`, `contact_emails`, `assigned_emails`, -`team_names`) must resolve within the caller's org, and `contact_emails` only -among the contacts the importer may open; missing references are reported as -row errors rather than silently dropped. Tags are auto-created at -commit if not present. +`team_names`) must resolve within the caller's org, and `account_name` and +`contact_emails` only among the records the importer may open; missing +references are reported as row errors rather than silently dropped. Tags are +auto-created at commit if not present. All reference lookups are bulk-prefetched once per call (one SELECT per reference type), not per-row, a 5000-row file with five reference columns @@ -27,7 +27,8 @@ from typing import Any, Iterable from django.db import transaction from django.db.models.functions import Lower -from accounts.models import Account +from accounts.access import visible_accounts_qs +from cases.access import visible_cases_qs from cases.models import Case from common.models import Profile, Tags, Teams from common.utils import CASE_TYPE, PRIORITY_CHOICE, STATUS_CHOICE @@ -280,19 +281,27 @@ def _build_ref_maps(parsed: list[tuple[int, dict[str, str]]], org, profile) -> _ for team in _split_multi(record.get("team_names", "")): team_names.add(team.lower()) + # Only tickets the importer may open. Ticket names carry no DB constraint, + # so this check is a courtesy against importing the same ticket twice, not + # a rule the database would enforce. Checked across the whole org it was + # an oracle: "A ticket with this name already exists" for a name held only + # by a ticket the importer cannot open confirmed that ticket existed. existing_case_names: set[str] = set() if candidate_names: existing_case_names = set( - Case.objects.filter(org=org) + visible_cases_qs(profile) .annotate(name_lower=Lower("name")) .filter(name_lower__in=candidate_names) .values_list("name_lower", flat=True) ) + # Only accounts the importer may open, for the reason given for contacts + # below: "No account named ..." for a name nobody holds and a valid row + # for one held by a hidden account told the importer the hidden one existed. accounts: dict[str, str] = {} if account_names: for pk, name_lower in ( - Account.objects.filter(org=org) + visible_accounts_qs(profile, profile.user) .annotate(name_lower=Lower("name")) .filter(name_lower__in=account_names) .values_list("id", "name_lower") @@ -416,7 +425,11 @@ def _validate_and_build( account_id = refs.accounts.get(account_name.lower()) if account_id is None: errors.append( - RowError(idx, "account_name", f"No account named '{account_name}'") + RowError( + idx, + "account_name", + f"No account you can open is named '{account_name}'", + ) ) contact_ids: list[str] = [] diff --git a/backend/cases/tests/test_case_account_visibility.py b/backend/cases/tests/test_case_account_visibility.py new file mode 100644 index 00000000..34c26429 --- /dev/null +++ b/backend/cases/tests/test_case_account_visibility.py @@ -0,0 +1,249 @@ +"""A member may link a ticket only to an account they can open (D38). + +`validate_account` refused another org's account and nothing else, so a member +could attach a ticket to any account in the org, including one the account +detail view would answer 403 for. The ticket CSV import went further: it +resolved `account_name` across the whole org, so its "No account named ..." +row error said which hidden accounts existed. + +Both now ask `accounts.access`: the API through `has_account_access`, the +import and the ticket form's picker through `visible_accounts_qs`. Every +refusal reads the same as an account that does not exist. +""" + +import csv +import io +import uuid + +import pytest +from django.core.files.uploadedfile import SimpleUploadedFile + +from accounts.models import Account +from cases.models import Case +from conftest import rls_org + +CASES_URL = "/api/cases/" +PREVIEW = "/api/cases/import/preview/" +COMMIT = "/api/cases/import/commit/" + + +def _account(org, name, created_by=None): + account = Account.objects.create(name=name, org=org) + Account.objects.filter(pk=account.pk).update(created_by=created_by) + return account + + +@pytest.fixture +def hidden(org_a, admin_user): + return _account(org_a, "Hidden Holdings", created_by=admin_user) + + +@pytest.fixture +def assigned(org_a, admin_user, user_profile): + account = _account(org_a, "Assigned Industries", created_by=admin_user) + account.assigned_to.add(user_profile) + return account + + +@pytest.fixture +def foreign(org_b, user_b): + with rls_org(org_b): + return _account(org_b, "Other Org Ltd", created_by=user_b) + + +@pytest.fixture +def superuser(regular_user): + """The `user_client` caller, with a plain USER profile, made superuser.""" + regular_user.is_superuser = True + regular_user.save(update_fields=["is_superuser"]) + return regular_user + + +@pytest.fixture +def importer(user_profile): + user_profile.has_sales_access = True + user_profile.save(update_fields=["has_sales_access"]) + return user_profile + + +def _create(client, name, account_id): + return client.post( + CASES_URL, + { + "name": name, + "status": "New", + "priority": "Normal", + "account": str(account_id), + }, + format="json", + ) + + +def _refusal(response): + assert response.status_code == 400, response.content + return response.json()["errors"]["account"] + + +@pytest.mark.django_db +class TestTicketApiAccountLink: + def test_member_may_link_an_account_they_are_assigned_to( + self, user_client, assigned + ): + response = _create(user_client, "Assigned link", assigned.id) + assert response.status_code == 200, response.content + assert Case.objects.get(name="Assigned link").account_id == assigned.id + + def test_member_may_link_an_account_they_created( + self, user_client, org_a, regular_user + ): + mine = _account(org_a, "Mine Co", created_by=regular_user) + response = _create(user_client, "Own link", mine.id) + assert response.status_code == 200, response.content + assert Case.objects.get(name="Own link").account_id == mine.id + + def test_hidden_account_reads_like_an_unknown_id(self, user_client, hidden): + hidden_refusal = _refusal(_create(user_client, "Hidden link", hidden.id)) + unknown_refusal = _refusal(_create(user_client, "Unknown link", uuid.uuid4())) + assert hidden_refusal == unknown_refusal == ["No such account."] + assert not Case.objects.filter( + name__in=["Hidden link", "Unknown link"] + ).exists() + + def test_admin_may_link_any_account_in_the_org(self, admin_client, hidden): + response = _create(admin_client, "Admin link", hidden.id) + assert response.status_code == 200, response.content + assert Case.objects.get(name="Admin link").account_id == hidden.id + + def test_superuser_may_link_any_account_in_the_org( + self, superuser, user_client, hidden + ): + response = _create(user_client, "Superuser link", hidden.id) + assert response.status_code == 200, response.content + assert Case.objects.get(name="Superuser link").account_id == hidden.id + + @pytest.mark.parametrize("caller", ["member", "admin", "superuser"]) + def test_other_org_account_is_refused_for_everyone( + self, caller, request, user_client, admin_client, foreign + ): + if caller == "superuser": + request.getfixturevalue("superuser") + client = admin_client if caller == "admin" else user_client + assert _refusal(_create(client, "Foreign link", foreign.id)) == [ + "No such account." + ] + assert not Case.objects.filter(name="Foreign link").exists() + + def test_account_cannot_be_changed_after_creation( + self, user_client, org_a, regular_user, user_profile, assigned, hidden + ): + case = Case.objects.create( + name="Existing", status="New", priority="Normal", org=org_a + ) + Case.objects.filter(pk=case.pk).update(created_by=regular_user) + case.assigned_to.add(user_profile) + for target in (assigned, hidden): + response = user_client.patch( + f"{CASES_URL}{case.id}/", {"account": str(target.id)}, format="json" + ) + assert response.status_code == 200, response.content + case.refresh_from_db() + assert case.account_id is None + + def test_picker_offers_exactly_what_the_save_path_accepts( + self, user_client, assigned, hidden + ): + offered = { + row["id"] for row in user_client.get(CASES_URL).json()["accounts_list"] + } + assert str(assigned.id) in offered + assert str(hidden.id) not in offered + + def test_picker_offers_a_superuser_every_account( + self, superuser, user_client, assigned, hidden + ): + offered = { + row["id"] for row in user_client.get(CASES_URL).json()["accounts_list"] + } + assert {str(assigned.id), str(hidden.id)} <= offered + + +def _csv(rows): + buf = io.StringIO() + writer = csv.writer(buf) + writer.writerow(["name", "status", "priority", "account_name"]) + writer.writerows(rows) + return SimpleUploadedFile( + "tickets.csv", buf.getvalue().encode("utf-8"), content_type="text/csv" + ) + + +def _messages(body): + return {e["row"]: e["message"] for e in body["errors"]} + + +@pytest.mark.django_db +class TestTicketImportAccountVisibility: + def test_hidden_and_missing_accounts_get_the_same_error( + self, user_client, importer, hidden + ): + response = user_client.post( + PREVIEW, + { + "file": _csv( + [ + ["Hidden", "New", "High", "Hidden Holdings"], + ["Missing", "New", "High", "Nobody Inc"], + ] + ) + }, + format="multipart", + ) + assert response.status_code == 200, response.content + body = response.json() + assert body["summary"]["valid"] == 0 + messages = _messages(body) + assert messages[1].replace("Hidden Holdings", "NAME") == messages[2].replace( + "Nobody Inc", "NAME" + ) + assert {e["field"] for e in body["errors"]} == {"account_name"} + + def test_visible_account_resolves_case_insensitively( + self, user_client, importer, assigned + ): + response = user_client.post( + COMMIT, + {"file": _csv([["Imported", "New", "High", "assigned INDUSTRIES"]])}, + format="multipart", + ) + assert response.status_code == 200, response.content + assert response.json()["created"] == 1 + assert Case.objects.get(name="Imported").account_id == assigned.id + + def test_commit_refuses_a_hidden_account(self, user_client, importer, hidden): + response = user_client.post( + COMMIT, + {"file": _csv([["Sneaky", "New", "High", "Hidden Holdings"]])}, + format="multipart", + ) + assert response.json()["created"] == 0 + assert not Case.objects.filter(name="Sneaky").exists() + + def test_admin_resolves_an_account_a_member_cannot(self, admin_client, hidden): + response = admin_client.post( + COMMIT, + {"file": _csv([["By admin", "New", "High", "Hidden Holdings"]])}, + format="multipart", + ) + assert response.json()["created"] == 1, response.content + assert Case.objects.get(name="By admin").account_id == hidden.id + + def test_superuser_resolves_an_account_a_member_cannot( + self, superuser, user_client, importer, hidden + ): + response = user_client.post( + COMMIT, + {"file": _csv([["By superuser", "New", "High", "Hidden Holdings"]])}, + format="multipart", + ) + assert response.json()["created"] == 1, response.content + assert Case.objects.get(name="By superuser").account_id == hidden.id diff --git a/backend/cases/tests/test_case_name_duplicate_visibility.py b/backend/cases/tests/test_case_name_duplicate_visibility.py new file mode 100644 index 00000000..339e4a61 --- /dev/null +++ b/backend/cases/tests/test_case_name_duplicate_visibility.py @@ -0,0 +1,134 @@ +"""A ticket-name clash is judged among tickets the caller can open (D48). + +Ticket names carry no DB constraint (``Case.name`` is a plain ``CharField`` +with no unique index or ``UniqueConstraint``), so refusing a duplicate name is +a courtesy against filing the same ticket twice. Both places that offered it +checked the whole org: the CSV import answered "A ticket with this name +already exists" and ``POST /api/cases/`` answered "Case already exists with +this name" for a name held only by a ticket the caller cannot open, which +confirmed that ticket existed. Both now look only at ``visible_cases_qs``. +""" + +import csv +import io + +import pytest +from django.core.files.uploadedfile import SimpleUploadedFile + +from cases.models import Case + +PREVIEW = "/api/cases/import/preview/" +IMPORT_CLASH = "A ticket with this name already exists" +CREATE_CLASH = "Case already exists with this name" + + +def _csv(names): + buf = io.StringIO() + writer = csv.writer(buf) + writer.writerow(["name", "status", "priority"]) + for name in names: + writer.writerow([name, "New", "High"]) + return SimpleUploadedFile( + "tickets.csv", buf.getvalue().encode("utf-8"), content_type="text/csv" + ) + + +@pytest.fixture +def importer(user_profile): + """A member with sales access, which `can_mass_import` admits.""" + user_profile.has_sales_access = True + user_profile.save(update_fields=["has_sales_access"]) + return user_profile + + +@pytest.fixture +def hidden(org_a, admin_user): + return Case.objects.create( + name="Hidden outage", + status="New", + priority="High", + org=org_a, + created_by=admin_user, + ) + + +@pytest.fixture +def mine(org_a, regular_user): + return Case.objects.create( + name="My outage", + status="New", + priority="High", + org=org_a, + created_by=regular_user, + ) + + +def _preview_errors(client, names): + response = client.post(PREVIEW, {"file": _csv(names)}, format="multipart") + assert response.status_code == 200, response.content + return response.json() + + +@pytest.mark.django_db +class TestImportNameClash: + def test_hidden_ticket_name_is_not_reported(self, user_client, importer, hidden): + body = _preview_errors(user_client, ["hidden OUTAGE"]) + assert body["errors"] == [] + assert body["summary"]["valid"] == 1 + + def test_visible_ticket_name_is_reported(self, user_client, importer, mine): + body = _preview_errors(user_client, ["my outage"]) + assert body["summary"]["valid"] == 0 + assert [(e["field"], e["message"]) for e in body["errors"]] == [ + ("name", IMPORT_CLASH) + ] + + def test_admin_sees_every_clash_in_the_org(self, admin_client, hidden): + body = _preview_errors(admin_client, ["Hidden outage"]) + assert [e["message"] for e in body["errors"]] == [IMPORT_CLASH] + + +def _create(client, name): + return client.post( + "/api/cases/", + {"name": name, "status": "New", "priority": "High"}, + format="json", + ) + + +@pytest.mark.django_db +class TestCreateNameClash: + def test_hidden_ticket_name_does_not_refuse_the_create( + self, user_client, org_a, hidden + ): + response = _create(user_client, "Hidden outage") + assert response.status_code == 200, response.content + assert Case.objects.filter(org=org_a, name="Hidden outage").count() == 2 + + def test_visible_ticket_name_refuses_the_create(self, user_client, org_a, mine): + response = _create(user_client, "MY OUTAGE") + assert response.status_code == 400, response.content + assert response.json()["errors"]["name"] == [CREATE_CLASH] + assert Case.objects.filter(org=org_a, name__iexact="my outage").count() == 1 + + def test_rename_onto_a_visible_name_is_refused( + self, user_client, org_a, regular_user, mine + ): + other = Case.objects.create( + name="Second", + status="New", + priority="High", + org=org_a, + created_by=regular_user, + ) + response = user_client.patch( + f"/api/cases/{other.id}/", {"name": "My outage"}, format="json" + ) + assert response.status_code == 400, response.content + assert response.json()["errors"]["name"] == [CREATE_CLASH] + + def test_keeping_its_own_name_is_not_a_clash(self, user_client, mine): + response = user_client.patch( + f"/api/cases/{mine.id}/", {"name": "My outage"}, format="json" + ) + assert response.status_code == 200, response.content diff --git a/backend/cases/tests/test_import_contact_visibility.py b/backend/cases/tests/test_import_contact_visibility.py index bcb21b80..8a43e7a8 100644 --- a/backend/cases/tests/test_import_contact_visibility.py +++ b/backend/cases/tests/test_import_contact_visibility.py @@ -1,6 +1,6 @@ """The ticket import preview does not reveal whether a contact exists. -`_can_import` admits members with `has_sales_access`, not only admins. The +`can_mass_import` admits members with `has_sales_access`, not only admins. The preview resolved `contact_emails` across the whole org, so such a member got "No contact with email ..." for an address nobody holds and a valid row for one held by a contact they cannot open: an existence oracle for any address. diff --git a/backend/cases/tests/test_kanban_move_close_gate.py b/backend/cases/tests/test_kanban_move_close_gate.py new file mode 100644 index 00000000..2e49bd0b --- /dev/null +++ b/backend/cases/tests/test_kanban_move_close_gate.py @@ -0,0 +1,183 @@ +"""A board move into Closed takes the close gate PATCH takes (D49). + +`CaseMoveView.patch` set ``status="Closed"`` itself, directly or through a +stage's ``maps_to_status``, and skipped the gate `CaseCreateSerializer.validate` +applies on the edit path. With a ``pre_close`` approval rule armed, PATCH +refused the close and a drag on the board closed the ticket anyway, with no +approval and no ``closed_on``. Both paths now call +``cases.approvals.close_refusal``: a refused move answers what PATCH answers +and writes nothing, an allowed one stamps ``closed_on``, and moving back out of +Closed clears what PATCH clears. +""" + +import datetime + +import pytest +from django.utils import timezone + +from cases.approvals import Approval, ApprovalRule +from cases.models import Case, CasePipeline, CaseStage + + +def _move(client, case, **body): + return client.patch(f"/api/cases/{case.id}/move/", body, format="json") + + +def _patch(client, case, **body): + return client.patch(f"/api/cases/{case.id}/", body, format="json") + + +def _case(org, user, name="Printer on fire", **fields): + fields.setdefault("status", "New") + return Case.objects.create( + name=name, priority="High", org=org, created_by=user, **fields + ) + + +@pytest.fixture +def rule(org_a): + return ApprovalRule.objects.create( + org=org_a, name="High needs sign-off", is_active=True, match_priority="High" + ) + + +@pytest.fixture +def closing_stage(org_a, admin_user): + pipeline = CasePipeline.objects.create( + name="Support", org=org_a, created_by=admin_user + ) + return CaseStage.objects.create( + pipeline=pipeline, + name="Resolved", + order=1, + stage_type="closed", + maps_to_status="Closed", + org=org_a, + created_by=admin_user, + ) + + +def _snapshot(case): + case.refresh_from_db() + return ( + case.status, + case.stage_id, + case.closed_on, + case.resolved_at, + case.kanban_order, + ) + + +@pytest.mark.django_db +class TestRefusedLikePatch: + def test_status_move_is_refused_with_the_patch_answer( + self, admin_client, admin_profile, admin_user, org_a, rule + ): + case = _case(org_a, admin_user) + before = _snapshot(case) + + patched = _patch( + admin_client, + case, + status="Closed", + closed_on=timezone.localdate().isoformat(), + ) + moved = _move(admin_client, case, status="Closed") + + assert patched.status_code == 400, patched.content + assert moved.status_code == patched.status_code, moved.content + assert moved.json() == patched.json() + assert moved.json()["errors"] == { + "status": [ + "An approval is required before this case can be closed " + "(rule: High needs sign-off)." + ] + } + assert _snapshot(case) == before + + def test_stage_move_that_maps_to_closed_is_refused( + self, admin_client, admin_profile, admin_user, org_a, rule, closing_stage + ): + case = _case(org_a, admin_user) + before = _snapshot(case) + + moved = _move(admin_client, case, stage_id=str(closing_stage.id)) + + assert moved.status_code == 400, moved.content + assert list(moved.json()["errors"]) == ["status"] + assert _snapshot(case) == before + + +@pytest.mark.django_db +class TestAllowedClose: + def test_approved_close_goes_through_and_stamps_closed_on( + self, admin_client, admin_profile, admin_user, org_a, rule + ): + case = _case(org_a, admin_user) + Approval.objects.create( + org=org_a, + case=case, + rule=rule, + requested_by=admin_profile, + state="approved", + ) + + moved = _move(admin_client, case, status="Closed") + + assert moved.status_code == 200, moved.content + case.refresh_from_db() + assert case.status == "Closed" + assert case.closed_on == timezone.localdate() + assert case.resolved_at is not None + + def test_close_without_a_rule_stamps_closed_on( + self, admin_client, admin_profile, admin_user, org_a, closing_stage + ): + case = _case(org_a, admin_user) + + moved = _move(admin_client, case, stage_id=str(closing_stage.id)) + + assert moved.status_code == 200, moved.content + case.refresh_from_db() + assert (case.status, case.stage_id) == ("Closed", closing_stage.id) + assert case.closed_on == timezone.localdate() + + def test_a_case_already_closed_is_not_re_approved( + self, admin_client, admin_profile, admin_user, org_a, rule + ): + closed_on = timezone.localdate() - datetime.timedelta(days=3) + case = _case(org_a, admin_user, status="Closed", closed_on=closed_on) + + moved = _move(admin_client, case, status="Closed") + + assert moved.status_code == 200, moved.content + case.refresh_from_db() + assert case.closed_on == closed_on + + +@pytest.mark.django_db +def test_reopen_by_move_clears_what_patch_clears( + admin_client, admin_profile, admin_user, org_a +): + closed_on = timezone.localdate() - datetime.timedelta(days=2) + by_move = _case(org_a, admin_user, "A", status="Closed", closed_on=closed_on) + by_patch = _case(org_a, admin_user, "B", status="Closed", closed_on=closed_on) + for case in (by_move, by_patch): + case.refresh_from_db() + assert case.resolved_at is not None + + assert _move(admin_client, by_move, status="Pending").status_code == 200 + assert _patch(admin_client, by_patch, status="Pending").status_code == 200 + + for case in (by_move, by_patch): + case.refresh_from_db() + assert (by_move.status, by_move.closed_on, by_move.resolved_at) == ( + by_patch.status, + by_patch.closed_on, + by_patch.resolved_at, + ) + assert (by_move.status, by_move.closed_on, by_move.resolved_at) == ( + "Pending", + None, + None, + ) diff --git a/backend/cases/tests/test_kanban_move_write_rule.py b/backend/cases/tests/test_kanban_move_write_rule.py new file mode 100644 index 00000000..c8002842 --- /dev/null +++ b/backend/cases/tests/test_kanban_move_write_rule.py @@ -0,0 +1,144 @@ +"""The board move takes the ticket's write rule, exactly (D42). + +`CaseMoveView.patch` rewrites a ticket's status, stage and kanban order. It used +to gate on ``is_org_admin(...) or request.user.is_superuser`` plus creator or +assignee, while `cases.access` gives a Django superuser nothing and +`CaseDetailView.patch` refuses them. So a superuser who is a plain member could +move, through the board, any ticket they could not edit through the detail +endpoint. These tests pin the move to `assert_case_write_access`. +""" + +import pytest + +from cases.models import Case, CaseWatcher +from conftest import rls_org + + +def _move(client, case, status="Assigned"): + return client.patch( + f"/api/cases/{case.id}/move/", {"status": status}, format="json" + ) + + +@pytest.fixture +def superuser(regular_user): + """The `user_client` caller, still a plain USER profile, made superuser.""" + regular_user.is_superuser = True + regular_user.save(update_fields=["is_superuser"]) + return regular_user + + +@pytest.fixture +def others_case(admin_user, org_a): + """A ticket the `user_client` caller neither created nor was handed.""" + return Case.objects.create( + name="Someone else's ticket", + status="New", + priority="Normal", + org=org_a, + created_by=admin_user, + ) + + +def _assert_unchanged(case): + case.refresh_from_db() + assert case.status == "New" + + +class TestSuperuserPlainMember: + def test_refused_on_a_ticket_they_cannot_edit( + self, superuser, user_client, others_case + ): + assert ( + user_client.patch( + f"/api/cases/{others_case.id}/", {"status": "Assigned"}, format="json" + ).status_code + == 403 + ) + response = _move(user_client, others_case) + assert response.status_code == 403, response.content + _assert_unchanged(others_case) + + def test_allowed_on_a_ticket_they_created( + self, superuser, user_client, regular_user, org_a + ): + case = Case.objects.create( + name="Mine", + status="New", + priority="Normal", + org=org_a, + created_by=regular_user, + ) + response = _move(user_client, case) + assert response.status_code == 200, response.content + case.refresh_from_db() + assert case.status == "Assigned" + + def test_allowed_on_a_ticket_they_are_assigned( + self, superuser, user_client, user_profile, others_case + ): + others_case.assigned_to.add(user_profile) + response = _move(user_client, others_case) + assert response.status_code == 200, response.content + others_case.refresh_from_db() + assert others_case.status == "Assigned" + + +class TestOrdinaryRoles: + def test_admin_may_move_any_ticket(self, admin_client, regular_user, org_a): + case = Case.objects.create( + name="A member's ticket", + status="New", + priority="Normal", + org=org_a, + created_by=regular_user, + ) + response = _move(admin_client, case) + assert response.status_code == 200, response.content + case.refresh_from_db() + assert case.status == "Assigned" + + def test_member_with_write_access_may_move( + self, user_client, user_profile, others_case + ): + others_case.assigned_to.add(user_profile) + response = _move(user_client, others_case) + assert response.status_code == 200, response.content + others_case.refresh_from_db() + assert others_case.status == "Assigned" + + def test_member_without_write_access_is_refused(self, user_client, others_case): + response = _move(user_client, others_case) + assert response.status_code == 403, response.content + _assert_unchanged(others_case) + + def test_watcher_reads_but_may_not_move( + self, user_client, user_profile, others_case, org_a + ): + """Watching is read access, not write access, on the board as elsewhere.""" + CaseWatcher.objects.create(case=others_case, profile=user_profile, org=org_a) + assert user_client.get(f"/api/cases/{others_case.id}/").status_code == 200 + response = _move(user_client, others_case) + assert response.status_code == 403, response.content + _assert_unchanged(others_case) + + +@pytest.mark.parametrize("caller", ["member", "admin", "superuser"]) +def test_another_orgs_ticket_is_404_for_everyone( + caller, request, user_client, admin_client, user_b, org_b +): + if caller == "superuser": + request.getfixturevalue("superuser") + client = admin_client if caller == "admin" else user_client + with rls_org(org_b): + foreign = Case.objects.create( + name="Foreign", + status="New", + priority="Normal", + org=org_b, + created_by=user_b, + ) + response = _move(client, foreign) + assert response.status_code == 404, response.content + with rls_org(org_b): + _assert_unchanged(foreign) diff --git a/backend/cases/tests/test_merged_redirect_read_rule.py b/backend/cases/tests/test_merged_redirect_read_rule.py new file mode 100644 index 00000000..0c68c3b7 --- /dev/null +++ b/backend/cases/tests/test_merged_redirect_read_rule.py @@ -0,0 +1,46 @@ +"""A merged ticket's redirect is behind the read rule (D45). + +The ticket detail GET answered a merged duplicate's redirect before checking +read access, so any member got the duplicate's name and the id of the ticket +it was merged into. +""" + +import pytest + +from cases.models import Case + + +@pytest.fixture +def merged_pair(admin_user, org_a): + target = Case.objects.create( + name="Kept ticket", + status="New", + priority="Normal", + org=org_a, + created_by=admin_user, + ) + duplicate = Case.objects.create( + name="Secret duplicate", + status="Closed", + priority="Normal", + org=org_a, + created_by=admin_user, + merged_into=target, + ) + return duplicate, target + + +class TestMergedRedirect: + def test_member_who_cannot_open_it_learns_nothing(self, user_client, merged_pair): + duplicate, target = merged_pair + response = user_client.get(f"/api/cases/{duplicate.id}/") + assert response.status_code == 403 + body = response.content.decode() + assert "Secret duplicate" not in body + assert str(target.id) not in body + + def test_reader_gets_the_redirect(self, admin_client, merged_pair): + duplicate, target = merged_pair + response = admin_client.get(f"/api/cases/{duplicate.id}/") + assert response.status_code == 200 + assert response.json()["redirect_to"] == str(target.id) diff --git a/backend/cases/tests/test_parent_child.py b/backend/cases/tests/test_parent_child.py index 38ce41d7..314f2424 100644 --- a/backend/cases/tests/test_parent_child.py +++ b/backend/cases/tests/test_parent_child.py @@ -89,7 +89,7 @@ class TestCleanGuards: @pytest.mark.django_db class TestSerializer: def test_parent_summary_and_child_count(self, admin_user, admin_profile, org_a): - from cases.serializer import CaseSerializer + from cases.serializer import CaseSerializer, parent_access_context root = _make_case(org_a, admin_user, name="Root", is_problem=True) _make_case(org_a, admin_user, name="C1", parent=root) @@ -103,11 +103,14 @@ class TestSerializer: assert data["is_problem"] is True c1 = root.children.first() - c1_data = CaseSerializer(c1).data + c1_data = CaseSerializer( + c1, context=parent_access_context(admin_profile, [c1]) + ).data assert c1_data["parent_summary"] == { "id": str(root.id), "name": root.name, "status": root.status, + "restricted": False, } assert c1_data["child_count"] == 0 diff --git a/backend/cases/tests/test_parent_summary_access.py b/backend/cases/tests/test_parent_summary_access.py new file mode 100644 index 00000000..fd451579 --- /dev/null +++ b/backend/cases/tests/test_parent_summary_access.py @@ -0,0 +1,230 @@ +"""`parent_summary` follows the ticket read rule on the parent (D51). + +`CaseSerializer.get_parent_summary` used to return the parent's id, name and +status to anybody who could read the child. Reading a child is not reading its +parent: a member assigned one sub-ticket learned the subject of a ticket the +detail view answers 403 for. A parent the viewer cannot open now comes back as +`{id, name: None, status: None, restricted: True}`, the shape `/tree/` uses for +a hidden node. + +The create/update serializer had the matching write-side gap: `parent` was +checked by org alone, so a member could file a ticket under a parent they +cannot open, the same link `/link/` refuses, and learn from the difference +between a 400 and a 200 that the hidden ticket exists. + +`user_client` is a plain member (role USER) whose user is `regular_user`. +""" + +from __future__ import annotations + +import uuid + +import pytest +from crum import impersonate +from django.db import connection +from django.test.utils import CaptureQueriesContext + +from cases.models import Case, CaseWatcher +from cases.serializer import PARENT_NOT_FOUND, CaseSerializer +from conftest import rls_org + +SECRET = "Secret payroll ticket" +REDACTED = {"name": None, "status": None, "restricted": True} + + +def _case(org, creator, name, *, parent=None): + with impersonate(creator): + with rls_org(org): + return Case.objects.create( + name=name, status="New", priority="Normal", org=org, parent=parent + ) + + +def _row(resp, case): + return next(r for r in resp.json()["cases"] if r["id"] == str(case.id)) + + +@pytest.mark.django_db +class TestDetail: + def test_member_who_cannot_read_the_parent_gets_it_redacted( + self, user_client, admin_user, regular_user, org_a + ): + parent = _case(org_a, admin_user, SECRET) + child = _case(org_a, regular_user, "My sub-ticket", parent=parent) + # The parent really is hidden from this member. + assert user_client.get(f"/api/cases/{parent.id}/").status_code == 403 + + resp = user_client.get(f"/api/cases/{child.id}/") + assert resp.status_code == 200, resp.content + assert resp.json()["cases_obj"]["parent_summary"] == { + "id": str(parent.id), + **REDACTED, + } + assert SECRET.encode() not in resp.content + + def test_member_who_can_read_the_parent_gets_it_in_full( + self, user_client, admin_user, regular_user, user_profile, org_a + ): + parent = _case(org_a, admin_user, "Watched parent") + CaseWatcher.objects.create(case=parent, profile=user_profile, org=org_a) + child = _case(org_a, regular_user, "My sub-ticket", parent=parent) + + resp = user_client.get(f"/api/cases/{child.id}/") + assert resp.status_code == 200, resp.content + assert resp.json()["cases_obj"]["parent_summary"] == { + "id": str(parent.id), + "name": "Watched parent", + "status": "New", + "restricted": False, + } + + def test_admin_gets_every_parent_in_full( + self, admin_client, admin_user, regular_user, org_a + ): + parent = _case(org_a, regular_user, "Member's parent") + child = _case(org_a, admin_user, "Child", parent=parent) + + summary = admin_client.get(f"/api/cases/{child.id}/").json()["cases_obj"][ + "parent_summary" + ] + assert summary["name"] == "Member's parent" + assert summary["restricted"] is False + + def test_a_serializer_given_no_viewer_redacts(self, admin_user, org_a): + # Fail closed: a call site that forgets the context leaks nothing. + parent = _case(org_a, admin_user, SECRET) + child = _case(org_a, admin_user, "Child", parent=parent) + assert CaseSerializer(child).data["parent_summary"] == { + "id": str(parent.id), + **REDACTED, + } + + +@pytest.mark.django_db +class TestList: + def test_each_row_is_judged_on_its_own_parent( + self, user_client, admin_user, regular_user, user_profile, org_a + ): + hidden = _case(org_a, admin_user, SECRET) + mine = _case(org_a, regular_user, "My own parent") + under_hidden = _case(org_a, regular_user, "Under hidden", parent=hidden) + under_mine = _case(org_a, regular_user, "Under mine", parent=mine) + + resp = user_client.get("/api/cases/") + assert resp.status_code == 200, resp.content + assert _row(resp, under_hidden)["parent_summary"] == { + "id": str(hidden.id), + **REDACTED, + } + assert _row(resp, under_mine)["parent_summary"]["name"] == "My own parent" + assert _row(resp, under_mine)["parent_summary"]["restricted"] is False + assert SECRET.encode() not in resp.content + + def test_parents_cost_one_query_per_page_not_one_per_row( + self, user_client, admin_user, regular_user, org_a + ): + # Every case exists before the first request, so both requests list + # the same rows; the readable parents are the member's own tickets and + # are on the list either way. Only the links change in between. + children = [_case(org_a, regular_user, f"Child {i}") for i in range(6)] + parents = [ + _case(org_a, admin_user if i % 2 else regular_user, f"Parent {i}") + for i in range(6) + ] + with CaptureQueriesContext(connection) as unlinked: + assert user_client.get("/api/cases/").status_code == 200 + + for child, parent in zip(children, parents): + Case.objects.filter(pk=child.pk).update(parent=parent) + + with CaptureQueriesContext(connection) as linked: + resp = user_client.get("/api/cases/") + assert resp.status_code == 200 + rows = [_row(resp, c)["parent_summary"] for c in children] + assert [r["restricted"] for r in rows] == [False, True] * 3 + # Six parents, half of them hidden, and one extra query for all of + # them: the batch lookup of which parent ids the member may read. + assert len(linked) - len(unlinked) <= 1 + + +@pytest.mark.django_db +class TestLinkingThroughTheTicketForm: + """`parent` on create and PATCH takes the rule `/link/` takes.""" + + def test_a_hidden_parent_is_refused_like_a_missing_one( + self, user_client, admin_user, regular_user, org_a + ): + hidden = _case(org_a, admin_user, SECRET) + mine = _case(org_a, regular_user, "Mine") + + refused = user_client.patch( + f"/api/cases/{mine.id}/", {"parent": str(hidden.id)}, format="json" + ) + missing = user_client.patch( + f"/api/cases/{mine.id}/", {"parent": str(uuid.uuid4())}, format="json" + ) + assert refused.status_code == missing.status_code == 400 + assert refused.json()["errors"]["parent"] == [PARENT_NOT_FOUND] + assert missing.json()["errors"]["parent"] == [PARENT_NOT_FOUND] + assert Case.objects.get(pk=mine.pk).parent_id is None + + def test_a_hidden_parent_is_refused_on_create(self, user_client, admin_user, org_a): + hidden = _case(org_a, admin_user, SECRET) + resp = user_client.post( + "/api/cases/", + { + "name": "New one", + "status": "New", + "priority": "Normal", + "parent": str(hidden.id), + }, + format="json", + ) + assert resp.status_code == 400, resp.content + assert SECRET.encode() not in resp.content + assert not Case.objects.filter(name="New one").exists() + + def test_a_readable_parent_is_accepted(self, user_client, regular_user, org_a): + parent = _case(org_a, regular_user, "My parent") + mine = _case(org_a, regular_user, "Mine") + resp = user_client.patch( + f"/api/cases/{mine.id}/", {"parent": str(parent.id)}, format="json" + ) + assert resp.status_code == 200, resp.content + assert Case.objects.get(pk=mine.pk).parent_id == parent.id + + def test_resending_the_current_hidden_parent_is_not_a_new_link( + self, user_client, admin_user, regular_user, org_a + ): + hidden = _case(org_a, admin_user, SECRET) + mine = _case(org_a, regular_user, "Mine", parent=hidden) + resp = user_client.patch( + f"/api/cases/{mine.id}/", + {"parent": str(hidden.id), "priority": "High"}, + format="json", + ) + assert resp.status_code == 200, resp.content + assert SECRET.encode() not in resp.content + assert Case.objects.get(pk=mine.pk).priority == "High" + + def test_admin_may_link_under_any_parent( + self, admin_client, admin_user, regular_user, org_a + ): + parent = _case(org_a, regular_user, "Member's") + child = _case(org_a, admin_user, "Admin's") + resp = admin_client.patch( + f"/api/cases/{child.id}/", {"parent": str(parent.id)}, format="json" + ) + assert resp.status_code == 200, resp.content + + +@pytest.mark.django_db +def test_another_orgs_case_is_refused_like_a_missing_one( + user_client, regular_user, case_b, org_a +): + mine = _case(org_a, regular_user, "Mine") + resp = user_client.patch( + f"/api/cases/{mine.id}/", {"parent": str(case_b.id)}, format="json" + ) + assert resp.status_code == 400, resp.content + assert resp.json()["errors"]["parent"] == [PARENT_NOT_FOUND] diff --git a/backend/cases/tests/test_parent_views_access.py b/backend/cases/tests/test_parent_views_access.py new file mode 100644 index 00000000..b3761ea0 --- /dev/null +++ b/backend/cases/tests/test_parent_views_access.py @@ -0,0 +1,348 @@ +"""Object-level authorization on the parent/child endpoints (D50). + +`CaseTreeView`, `CaseLinkParentView` and `CaseCloseWithChildrenView` used to +filter by org and nothing else. Any member could read the names of tickets they +cannot open through the tree, link or unlink any ticket, and close any ticket +together with its whole subtree, skipping the approval gate on the way. + +The rules they follow now are the ones in `cases.access`: + +* tree: read on the ticket asked about; unreadable nodes come back redacted; +* link: write on the child, read on the new parent (a hidden parent answers + like a missing one); unlink: write on the child; +* close with children: write on the ticket and on every descendant the + cascade would close, and `close_refusal` passes on each. All or nothing. + +`user_client` is a plain member (role USER). `admin_user` creates the tickets +that member is not supposed to reach. +""" + +from __future__ import annotations + +import uuid + +import pytest +from crum import impersonate +from django.utils import timezone + +from cases.approvals import ApprovalRule +from cases.models import Case, CaseWatcher +from common.models import Activity +from conftest import rls_org + + +def _case(org, creator, name, *, parent=None, priority="Normal", status="New"): + with impersonate(creator): + with rls_org(org): + return Case.objects.create( + name=name, + status=status, + priority=priority, + org=org, + parent=parent, + closed_on=timezone.localdate() if status == "Closed" else None, + ) + + +def _watch(case, profile): + CaseWatcher.objects.create(case=case, profile=profile, org=case.org) + + +def _statuses(*cases): + return [Case.objects.get(pk=c.pk).status for c in cases] + + +# --------------------------------------------------------------------------- # +# GET /api/cases//tree/ +# --------------------------------------------------------------------------- # + + +@pytest.mark.django_db +class TestTreeAccess: + def test_member_without_read_is_refused( + self, user_client, admin_user, user_profile, org_a + ): + root = _case(org_a, admin_user, "Admin root") + _case(org_a, admin_user, "Admin child", parent=root) + resp = user_client.get(f"/api/cases/{root.id}/tree/") + # The ticket detail GET answers a hidden in-org ticket 403 too. + assert resp.status_code == 403 + assert b"Admin child" not in resp.content + assert b"Admin root" not in resp.content + + def test_detail_get_answers_the_same(self, user_client, admin_user, org_a): + root = _case(org_a, admin_user, "Admin root") + assert user_client.get(f"/api/cases/{root.id}/").status_code == 403 + + def test_watcher_may_read_the_tree( + self, user_client, admin_user, user_profile, org_a + ): + root = _case(org_a, admin_user, "Watched root") + _watch(root, user_profile) + resp = user_client.get(f"/api/cases/{root.id}/tree/") + assert resp.status_code == 200, resp.content + assert resp.json()["root"]["name"] == "Watched root" + + def test_hidden_descendant_is_redacted( + self, user_client, admin_user, regular_user, org_a + ): + root = _case(org_a, regular_user, "My root") + hidden = _case(org_a, admin_user, "Secret payroll ticket", parent=root) + mine = _case(org_a, regular_user, "My grandchild", parent=hidden) + + resp = user_client.get(f"/api/cases/{mine.id}/tree/") + assert resp.status_code == 200, resp.content + assert b"Secret payroll ticket" not in resp.content + + body = resp.json() + assert body["root"]["name"] == "My root" + node = body["root"]["children"][0] + assert node["id"] == str(hidden.id) + assert node["restricted"] is True + assert node["name"] is None + assert "priority" not in node and "assigned_to" not in node + # Shape kept: the readable grandchild still sits under it. + assert node["children"][0]["name"] == "My grandchild" + assert "restricted" not in node["children"][0] + + def test_admin_sees_every_name(self, admin_client, regular_user, org_a): + root = _case(org_a, regular_user, "Member root") + _case(org_a, regular_user, "Member child", parent=root) + resp = admin_client.get(f"/api/cases/{root.id}/tree/") + assert resp.status_code == 200 + child = resp.json()["root"]["children"][0] + assert child["name"] == "Member child" + assert "restricted" not in child + + def test_other_org_is_404(self, org_b_client, admin_user, org_a): + root = _case(org_a, admin_user, "Org A root") + assert org_b_client.get(f"/api/cases/{root.id}/tree/").status_code == 404 + + +# --------------------------------------------------------------------------- # +# POST /api/cases//link/ +# --------------------------------------------------------------------------- # + + +@pytest.mark.django_db +class TestLinkAccess: + def test_member_cannot_link_a_ticket_they_cannot_write( + self, user_client, admin_user, regular_user, user_profile, org_a + ): + child = _case(org_a, admin_user, "Admin child") + # Watching grants read, not write. + _watch(child, user_profile) + parent = _case(org_a, regular_user, "My parent") + resp = user_client.post( + f"/api/cases/{child.id}/link/", {"parent_id": str(parent.id)}, format="json" + ) + assert resp.status_code == 403 + child.refresh_from_db() + assert child.parent_id is None + assert ( + not Activity.objects.filter(entity_id=child.id) + .filter(action="LINKED_PARENT") + .exists() + ) + + def test_member_cannot_unlink_a_ticket_they_cannot_write( + self, user_client, admin_user, org_a + ): + parent = _case(org_a, admin_user, "Admin parent") + child = _case(org_a, admin_user, "Admin child", parent=parent) + resp = user_client.post( + f"/api/cases/{child.id}/link/", {"parent_id": None}, format="json" + ) + assert resp.status_code == 403 + child.refresh_from_db() + assert child.parent_id == parent.id + + def test_hidden_parent_answers_like_a_missing_one( + self, user_client, admin_user, regular_user, org_a + ): + child = _case(org_a, regular_user, "My child") + hidden = _case(org_a, admin_user, "Secret parent") + hidden_resp = user_client.post( + f"/api/cases/{child.id}/link/", {"parent_id": str(hidden.id)}, format="json" + ) + missing_resp = user_client.post( + f"/api/cases/{child.id}/link/", + {"parent_id": str(uuid.uuid4())}, + format="json", + ) + assert hidden_resp.status_code == missing_resp.status_code == 400 + assert hidden_resp.json() == missing_resp.json() + assert b"Secret parent" not in hidden_resp.content + child.refresh_from_db() + assert child.parent_id is None + + def test_malformed_parent_id_is_a_missing_parent( + self, user_client, regular_user, org_a + ): + child = _case(org_a, regular_user, "My child") + resp = user_client.post( + f"/api/cases/{child.id}/link/", {"parent_id": "not-a-uuid"}, format="json" + ) + assert resp.status_code == 400 + assert "parent_id" in resp.json() + + def test_writer_links_under_a_readable_parent( + self, user_client, admin_user, regular_user, user_profile, org_a + ): + child = _case(org_a, regular_user, "My child") + parent = _case(org_a, admin_user, "Watched parent") + _watch(parent, user_profile) + resp = user_client.post( + f"/api/cases/{child.id}/link/", {"parent_id": str(parent.id)}, format="json" + ) + assert resp.status_code == 200, resp.content + child.refresh_from_db() + assert child.parent_id == parent.id + + def test_writer_unlinks(self, user_client, admin_user, regular_user, org_a): + parent = _case(org_a, admin_user, "Admin parent") + child = _case(org_a, regular_user, "My child", parent=parent) + resp = user_client.post( + f"/api/cases/{child.id}/link/", {"parent_id": None}, format="json" + ) + assert resp.status_code == 200, resp.content + child.refresh_from_db() + assert child.parent_id is None + + def test_admin_links_any_ticket(self, admin_client, regular_user, org_a): + child = _case(org_a, regular_user, "Member child") + parent = _case(org_a, regular_user, "Member parent") + resp = admin_client.post( + f"/api/cases/{child.id}/link/", {"parent_id": str(parent.id)}, format="json" + ) + assert resp.status_code == 200, resp.content + child.refresh_from_db() + assert child.parent_id == parent.id + + def test_other_org_is_404(self, org_b_client, admin_user, org_a): + child = _case(org_a, admin_user, "Org A child") + resp = org_b_client.post( + f"/api/cases/{child.id}/link/", {"parent_id": None}, format="json" + ) + assert resp.status_code == 404 + + +# --------------------------------------------------------------------------- # +# POST /api/cases//close-with-children/ +# --------------------------------------------------------------------------- # + + +def _close(client, case, cascade=True): + return client.post( + f"/api/cases/{case.id}/close-with-children/", + {"cascade": cascade, "resolution_comment": "done"}, + format="json", + ) + + +@pytest.mark.django_db +class TestCloseWithChildrenAccess: + def test_member_cannot_close_a_ticket_they_cannot_write( + self, user_client, admin_user, user_profile, org_a + ): + root = _case(org_a, admin_user, "Admin root") + _watch(root, user_profile) + child = _case(org_a, admin_user, "Admin child", parent=root) + resp = _close(user_client, root) + assert resp.status_code == 403 + assert _statuses(root, child) == ["New", "New"] + + def test_one_unwritable_descendant_refuses_the_whole_close( + self, user_client, admin_user, regular_user, org_a + ): + root = _case(org_a, regular_user, "My root") + mine = _case(org_a, regular_user, "My child", parent=root) + hidden = _case(org_a, admin_user, "Secret sibling", parent=root) + resp = _close(user_client, root) + assert resp.status_code == 400, resp.content + body = resp.json() + assert body["error"] is True + assert "not yours to close" in body["errors"] + assert b"Secret sibling" not in resp.content + assert _statuses(root, mine, hidden) == ["New", "New", "New"] + assert not Activity.objects.filter(action="PARENT_CLOSED_CASCADE").exists() + + def test_member_may_close_their_ticket_alone( + self, user_client, admin_user, regular_user, org_a + ): + # Without the cascade, the unwritable child is not touched and does + # not stand in the way. + root = _case(org_a, regular_user, "My root") + hidden = _case(org_a, admin_user, "Admin child", parent=root) + resp = _close(user_client, root, cascade=False) + assert resp.status_code == 200, resp.content + assert _statuses(root, hidden) == ["Closed", "New"] + + def test_approval_required_on_a_descendant_refuses( + self, admin_client, admin_user, org_a + ): + root = _case(org_a, admin_user, "Root", priority="Low") + gated = _case(org_a, admin_user, "Urgent child", parent=root, priority="Urgent") + ApprovalRule.objects.create( + name="Urgent needs sign-off", + org=org_a, + trigger_event="pre_close", + match_priority="Urgent", + ) + resp = _close(admin_client, root) + assert resp.status_code == 400, resp.content + message = resp.json()["errors"] + assert "approval" in message and "Urgent child" in message + assert _statuses(root, gated) == ["New", "New"] + + def test_approval_required_on_the_ticket_itself_refuses( + self, admin_client, admin_user, org_a + ): + root = _case(org_a, admin_user, "Urgent root", priority="Urgent") + ApprovalRule.objects.create( + name="Urgent needs sign-off", + org=org_a, + trigger_event="pre_close", + match_priority="Urgent", + ) + resp = _close(admin_client, root, cascade=False) + assert resp.status_code == 400, resp.content + assert "approval" in resp.json()["errors"]["status"][0] + assert _statuses(root) == ["New"] + + def test_allowed_close_stamps_every_ticket(self, user_client, regular_user, org_a): + root = _case(org_a, regular_user, "My root") + child = _case(org_a, regular_user, "My child", parent=root) + grandchild = _case(org_a, regular_user, "My grandchild", parent=child) + resp = _close(user_client, root) + assert resp.status_code == 200, resp.content + assert set(resp.json()["cascaded_case_ids"]) == { + str(child.id), + str(grandchild.id), + } + today = timezone.localdate() + for c in (root, child, grandchild): + c.refresh_from_db() + assert c.status == "Closed" + assert c.closed_on == today + assert c.resolved_at is not None + + def test_admin_closes_everybodys_subtree(self, admin_client, regular_user, org_a): + root = _case(org_a, regular_user, "Member root") + child = _case(org_a, regular_user, "Member child", parent=root) + resp = _close(admin_client, root) + assert resp.status_code == 200, resp.content + assert _statuses(root, child) == ["Closed", "Closed"] + + def test_stored_cycle_does_not_hang(self, admin_client, admin_user, org_a): + a = _case(org_a, admin_user, "A") + b = _case(org_a, admin_user, "B", parent=a) + Case.objects.filter(pk=a.pk).update(parent=b) + resp = _close(admin_client, a) + assert resp.status_code == 200, resp.content + assert resp.json()["cascaded_case_ids"] == [str(b.id)] + + def test_other_org_is_404(self, org_b_client, admin_user, org_a): + root = _case(org_a, admin_user, "Org A root") + assert _close(org_b_client, root).status_code == 404 + assert _statuses(root) == ["New"] diff --git a/backend/cases/tests/test_pipeline_counts_visibility.py b/backend/cases/tests/test_pipeline_counts_visibility.py new file mode 100644 index 00000000..4f2c9e42 --- /dev/null +++ b/backend/cases/tests/test_pipeline_counts_visibility.py @@ -0,0 +1,127 @@ +"""``case_count`` on a ticket pipeline counts only tickets the caller can open (D47). + +It used to count every case in the org, so a member whose board showed one +card read ``case_count: 2``, a number that included a ticket they cannot open. +It follows ``cases.access.visible_cases_qs`` now, the rule the board and the +list use, in all four places it appears: the pipeline list, the pipeline +detail, each of its nested stages, and the pipeline block on the board. The +same fix D9 made for lead pipelines. +""" + +import pytest +from django.db import connection +from django.test.utils import CaptureQueriesContext + +from cases.models import Case, CasePipeline, CaseStage, CaseWatcher +from conftest import rls_org + + +def _pipeline(org, user, name="Support"): + pipeline = CasePipeline.objects.create(name=name, org=org, created_by=user) + first = CaseStage.objects.create( + pipeline=pipeline, name="Triage", order=1, org=org, created_by=user + ) + second = CaseStage.objects.create( + pipeline=pipeline, name="Working", order=2, org=org, created_by=user + ) + return pipeline, first, second + + +def _case(org, user, name, stage): + return Case.objects.create( + name=name, + status="New", + priority="Normal", + org=org, + created_by=user, + stage=stage, + ) + + +@pytest.fixture +def seeded(admin_user, admin_profile, user_profile, org_a, org_b, user_b): + pipeline, first, _second = _pipeline(org_a, admin_user) + _case(org_a, admin_user, "Hidden from the member", first) + mine = _case(org_a, admin_user, "Handed to the member", first) + # Two assignees: a join on the M2M would count this ticket twice. + mine.assigned_to.add(user_profile, admin_profile) + # Another org's ticket, in another org's pipeline, never counted here. + with rls_org(org_b): + _b_pipeline, b_stage, _ = _pipeline(org_b, user_b, "Org B") + _case(org_b, user_b, "Org B ticket", b_stage) + return pipeline + + +def _counts(client, pipeline): + """case_count from the list, the detail, its first stage and the board.""" + (row,) = client.get("/api/cases/pipelines/").json()["pipelines"] + assert row["id"] == str(pipeline.id) + detail = client.get(f"/api/cases/pipelines/{pipeline.id}/").json() + board = client.get("/api/cases/kanban/", {"pipeline_id": str(pipeline.id)}).json() + assert [s["case_count"] for s in detail["stages"][1:]] == [0] + return ( + row["case_count"], + detail["case_count"], + detail["stages"][0]["case_count"], + board["pipeline"]["case_count"], + ) + + +@pytest.mark.django_db +class TestCasePipelineCountsFollowVisibility: + def test_member_counts_only_visible_cases(self, user_client, seeded): + assert _counts(user_client, seeded) == (1, 1, 1, 1) + + def test_admin_counts_every_case_in_the_org(self, admin_client, seeded): + assert _counts(admin_client, seeded) == (2, 2, 2, 2) + board = admin_client.get( + "/api/cases/kanban/", {"pipeline_id": str(seeded.id)} + ).json() + assert board["columns"][0]["case_count"] == 2 + assert board["pipeline"]["stage_count"] == 2 + + def test_member_board_column_and_pipeline_agree(self, user_client, seeded): + board = user_client.get( + "/api/cases/kanban/", {"pipeline_id": str(seeded.id)} + ).json() + assert board["columns"][0]["case_count"] == 1 + assert len(board["columns"][0]["cases"]) == 1 + assert board["pipeline"]["case_count"] == 1 + + def test_watcher_counts_the_ticket_they_follow( + self, user_client, user_profile, admin_user, org_a + ): + pipeline, first, _second = _pipeline(org_a, admin_user, "Watched") + watched = _case(org_a, admin_user, "Watched ticket", first) + CaseWatcher.objects.create(case=watched, profile=user_profile, org=org_a) + detail = user_client.get(f"/api/cases/pipelines/{pipeline.id}/").json() + assert detail["case_count"] == 1 + + def test_write_responses_carry_case_count(self, admin_client, admin_user, org_a): + created = admin_client.post( + "/api/cases/pipelines/", {"name": "Fresh"}, format="json" + ).json() + assert created["case_count"] == 0 + assert {s["case_count"] for s in created["stages"]} == {0} + + _pipeline_obj, first, _second = _pipeline(org_a, admin_user, "Other") + _case(org_a, admin_user, "Staged", first) + updated = admin_client.put( + f"/api/cases/stages/{first.id}/", {"name": "Renamed"}, format="json" + ).json() + assert updated["case_count"] == 1 + + def test_pipeline_list_query_count_does_not_grow_per_pipeline( + self, admin_client, admin_user, org_a, seeded + ): + with CaptureQueriesContext(connection) as one: + admin_client.get("/api/cases/pipelines/") + + for name in ("Second", "Third"): + _p, stage, _s = _pipeline(org_a, admin_user, name) + _case(org_a, admin_user, f"{name} ticket", stage) + with CaptureQueriesContext(connection) as three: + response = admin_client.get("/api/cases/pipelines/") + + assert len(response.json()["pipelines"]) == 3 + assert len(three.captured_queries) == len(one.captured_queries) diff --git a/backend/cases/tests/test_time_tracking.py b/backend/cases/tests/test_time_tracking.py index e5cfb916..2c503c39 100644 --- a/backend/cases/tests/test_time_tracking.py +++ b/backend/cases/tests/test_time_tracking.py @@ -39,13 +39,14 @@ def auto_stop_stale_timers(*args, **kwargs): restore_rls_context() -def _make_case(org, creator, name="Sample case"): +def _make_case(org, creator, name="Sample case", account=None): with impersonate(creator): return Case.objects.create( name=name, status="New", priority="Normal", org=org, + account=account, ) @@ -490,8 +491,8 @@ class TestInvoiceFromTimeEntries: def test_creates_draft_invoice( self, admin_client, admin_user, admin_profile, org_a ): - case = _make_case(org_a, admin_user) account = Account.objects.create(name="ACME", org=org_a) + case = _make_case(org_a, admin_user, account=account) e1 = _entry( case, admin_profile, @@ -530,8 +531,8 @@ class TestInvoiceFromTimeEntries: def test_mixed_currency_rejected( self, admin_client, admin_user, admin_profile, org_a ): - case = _make_case(org_a, admin_user) account = Account.objects.create(name="ACME", org=org_a) + case = _make_case(org_a, admin_user, account=account) e1 = _entry( case, admin_profile, @@ -561,8 +562,8 @@ class TestInvoiceFromTimeEntries: def test_non_billable_rejected( self, admin_client, admin_user, admin_profile, org_a ): - case = _make_case(org_a, admin_user) account = Account.objects.create(name="ACME", org=org_a) + case = _make_case(org_a, admin_user, account=account) e1 = _entry( case, admin_profile, diff --git a/backend/cases/unmerge_views.py b/backend/cases/unmerge_views.py index 50ef5bfd..3de648de 100644 --- a/backend/cases/unmerge_views.py +++ b/backend/cases/unmerge_views.py @@ -23,7 +23,7 @@ from rest_framework.views import APIView from cases.merge_views import _can_merge from cases.models import Case, EmailMessage -from cases.serializer import CaseSerializer +from cases.serializer import CaseSerializer, parent_access_context from cases.signals import _create_activity from common.models import Attachments, Comment from common.permissions import HasOrgContext @@ -187,7 +187,9 @@ class CaseUnmergeView(APIView): { "error": False, "message": "Tickets unmerged", - "source_case": CaseSerializer(source).data, + "source_case": CaseSerializer( + source, context=parent_access_context(request.profile, [source]) + ).data, }, status=status.HTTP_200_OK, ) diff --git a/backend/cases/views.py b/backend/cases/views.py index aff63c22..18e458a4 100644 --- a/backend/cases/views.py +++ b/backend/cases/views.py @@ -17,7 +17,7 @@ from rest_framework.permissions import IsAuthenticated from rest_framework.response import Response from rest_framework.views import APIView -from accounts.models import Account +from accounts.access import visible_accounts_qs from accounts.serializer import AccountPickerSerializer from cases import swagger_params from cases.access import ( @@ -39,6 +39,7 @@ from cases.serializer import ( CaseSerializer, EmailMessageSerializer, ReopenPolicySerializer, + parent_access_context, ) from cases.solution_serializers import SolutionSerializer from cases.tasks import send_email_to_assigned_user @@ -67,8 +68,7 @@ from common.utils import ( validate_attachment, ) from common.validators import date_param, payload_id_list, uuid_list_param, uuid_param -from contacts.access import replace_visible_contacts -from contacts.models import Contact +from contacts.access import replace_visible_contacts, visible_contacts_qs from contacts.serializer import ContactLinkSerializer, ContactPickerSerializer # A ticket is "open" while somebody still owes the customer something. The @@ -202,8 +202,12 @@ class CaseListView(APIView, LimitOffsetPagination): queryset = queryset.filter(merged_into__isnull=True).exclude( status="Duplicate" ) - accounts = Account.objects.filter(org=self.request.profile.org).order_by("-id") - contacts = Contact.objects.filter(org=self.request.profile.org).order_by("-id") + # The account read rule itself, which is what the save path accepts. + accounts = visible_accounts_qs( + self.request.profile, self.request.user + ).order_by("-id") + # The contact read rule itself, which is what the save path accepts. + contacts = visible_contacts_qs(self.request.profile).order_by("-id") profiles = Profile.objects.filter(is_active=True, org=self.request.profile.org) if not is_org_admin(self.request.profile): # Watcher allowance: a non-admin who is a watcher must still be @@ -214,14 +218,6 @@ class CaseListView(APIView, LimitOffsetPagination): queryset = queryset.filter( pk__in=visible_cases_qs(self.request.profile).values("pk") ) - accounts = accounts.filter( - Q(created_by=self.request.profile.user) - | Q(assigned_to=self.request.profile) - ).distinct() - contacts = contacts.filter( - Q(created_by=self.request.profile.user) - | Q(assigned_to=self.request.profile) - ).distinct() profiles = profiles.filter(role="ADMIN") queryset = apply_case_list_filters(queryset, params) @@ -245,7 +241,11 @@ class CaseListView(APIView, LimitOffsetPagination): ).count() results_cases = self.paginate_queryset(queryset, self.request, view=self) - cases = CaseSerializer(results_cases, many=True).data + cases = CaseSerializer( + results_cases, + many=True, + context=parent_access_context(self.request.profile, results_cases), + ).data if results_cases: offset = queryset.filter(id__gte=results_cases[-1].id).count() @@ -392,7 +392,10 @@ class CaseListView(APIView, LimitOffsetPagination): "error": False, "message": "Case Created Successfully", "id": str(cases_obj.id), - "cases_obj": CaseSerializer(cases_obj).data, + "cases_obj": CaseSerializer( + cases_obj, + context=parent_access_context(request.profile, [cases_obj]), + ).data, }, status=status.HTTP_200_OK, ) @@ -579,7 +582,11 @@ class CaseDetailView(APIView): ) def get(self, request, pk, format=None): self.cases = self.get_object(pk=pk) - # Merged duplicate → tell the client to redirect. JSON form (200) keeps + # Authorise before anything about the case leaves: the merge redirect + # below used to answer first, handing any member the name of a ticket + # they cannot open and the id of the one it was merged into. + assert_case_read_access(request.profile, self.cases) + # Merged duplicate: tell the client to redirect. JSON form (200) keeps # the SvelteKit route's error handling simple. The query param # `?show_merged=true` lets agents view the duplicate directly via # bookmark / list-view escape hatch. @@ -596,12 +603,11 @@ class CaseDetailView(APIView): }, status=status.HTTP_200_OK, ) - # Authorise before serialising: the old order built the response body - # for a case the requester was about to be refused. - assert_case_read_access(request.profile, self.cases) context = {} - context["cases_obj"] = CaseSerializer(self.cases).data + context["cases_obj"] = CaseSerializer( + self.cases, context=parent_access_context(request.profile, [self.cases]) + ).data # `comment_permission` used to be creator-or-admin while `post` below # accepted creator, admin *or assignee*. The flag told an assignee they @@ -762,7 +768,10 @@ class CaseDetailView(APIView): context.update( { - "cases_obj": CaseSerializer(self.cases_obj).data, + "cases_obj": CaseSerializer( + self.cases_obj, + context=parent_access_context(request.profile, [self.cases_obj]), + ).data, "attachments": AttachmentsSerializer(attachments, many=True).data, "comments": CommentSerializer( comments_qs.filter(is_internal=False), many=True diff --git a/backend/cases/watcher_views.py b/backend/cases/watcher_views.py index 69a3f769..c57383b5 100644 --- a/backend/cases/watcher_views.py +++ b/backend/cases/watcher_views.py @@ -95,7 +95,7 @@ class WatchingListView(APIView): permission_classes = (IsAuthenticated, HasOrgContext) def get(self, request, *args, **kwargs): - from cases.serializer import CaseSerializer + from cases.serializer import CaseSerializer, parent_access_context from cases.views import apply_case_list_filters cases = ( @@ -106,7 +106,11 @@ class WatchingListView(APIView): cases = apply_case_list_filters(cases, request.query_params) return Response( { - "cases": CaseSerializer(cases, many=True).data, + "cases": CaseSerializer( + cases, + many=True, + context=parent_access_context(request.profile, cases), + ).data, "count": cases.count(), } ) diff --git a/backend/common/packs/schema.py b/backend/common/packs/schema.py index 17cdf46d..e4bda02d 100644 --- a/backend/common/packs/schema.py +++ b/backend/common/packs/schema.py @@ -240,6 +240,15 @@ def _validate_stage( raise PackValidationError( f"{where}: maps_to_status {maps_to_status!r} is not valid" ) + if "win_probability" in stage: + # Copied onto Lead.probability by the board move, which has a 0-100 + # CheckConstraint, and LeadStage carries the same one. bool is an int + # subclass, so it is refused by name. + wp = stage["win_probability"] + if isinstance(wp, bool) or not isinstance(wp, int) or not 0 <= wp <= 100: + raise PackValidationError( + f"{where}: win_probability {wp!r} must be an integer from 0 to 100" + ) def _validate_pipeline( diff --git a/backend/common/permissions.py b/backend/common/permissions.py index fe807f87..b641d89a 100644 --- a/backend/common/permissions.py +++ b/backend/common/permissions.py @@ -43,6 +43,22 @@ def is_org_admin(profile): return getattr(profile, "role", None) == "ADMIN" +def can_mass_import(profile): + """Whether ``profile`` may bulk-create records through a CSV import. + + Org admins, Django superusers, and members granted ``has_sales_access``. + Everyone else can still create records one at a time; the import is the + mass-create surface, so it is gated more narrowly. One rule for the + contact, ticket and lead importers, which used to carry three copies of + it, none of which admitted a superuser. + """ + if profile is None: + return False + if is_org_admin(profile) or profile.user.is_superuser: + return True + return bool(profile.has_sales_access) + + class HasOrgContext(permissions.BasePermission): """ Permission class that requires valid organization context. diff --git a/backend/common/search_views.py b/backend/common/search_views.py index 75b2ec82..18937248 100644 --- a/backend/common/search_views.py +++ b/backend/common/search_views.py @@ -4,15 +4,18 @@ ONE endpoint, deliberately. The org comes from the JWT (`request.profile.org`) exactly once and every queryset is filtered by it; a per-model fan-out from the browser would be as many chances to read another tenant's rows. -Each type also honours the SAME read visibility as its own list view, so search -can never surface a record the caller could not open from the list: +Each type also honours the SAME read rule as its own detail view, by calling +that module's visibility helper rather than restating it, so search can never +surface a record the caller could not open, or hide one they could: -* leads / opportunities / invoices, admin (role ``ADMIN`` or a Django - superuser) sees the whole org; everyone else sees only what they created or - were assigned. -* accounts / contacts, admin (role ``ADMIN`` or the org-admin flag) sees the - whole org; everyone else sees created-or-assigned. -* tickets. Reuse ``visible_cases_qs`` (adds the watcher clause). +* leads ``visible_leads_qs``, deals ``visible_deals_qs``, accounts + ``visible_accounts_qs``, invoices ``visible_invoices_qs``: admins and Django + superusers see the whole org, everyone else what they created or were + assigned. +* contacts ``visible_contacts_qs``: the same, plus contacts at an account the + caller is assigned to. +* tickets ``visible_cases_qs``: admins, creator, assignees and watchers. No + superuser clause, because the ticket detail view has none. * knowledge-base articles. Org-wide, every member reads them. Matching stays on the server; the whole record set never reaches the browser. @@ -23,14 +26,14 @@ from rest_framework.permissions import IsAuthenticated from rest_framework.response import Response from rest_framework.views import APIView -from accounts.models import Account -from cases.access import is_org_admin, visible_cases_qs +from accounts.access import visible_accounts_qs +from cases.access import visible_cases_qs from cases.models import Solution from common.permissions import HasOrgContext -from contacts.models import Contact -from invoices.models import Invoice -from leads.models import Lead -from opportunity.models import Opportunity +from contacts.access import visible_contacts_qs +from invoices.permissions import visible_invoices_qs +from leads.access import visible_leads_qs +from opportunity.access import visible_deals_qs # Rows per type. Small on purpose. The palette shows a handful per group and # the point is the fastest match, not an exhaustive report. @@ -39,30 +42,6 @@ PER_TYPE = 6 MIN_QUERY = 2 -def _own_filter(profile): - """The created-or-assigned clause every assignable list view uses. - - ``created_by`` is a FK to ``User`` (not Profile), so it is compared against - ``profile.user``: comparing it to the Profile is the silent always-False - bug this codebase has hit before. - """ - return Q(created_by=profile.user) | Q(assigned_to=profile) - - -def _scope_orgadmin(qs, profile): - """accounts / contacts rule: admin = role ADMIN or the org-admin flag.""" - if is_org_admin(profile): - return qs - return qs.filter(_own_filter(profile)).distinct() - - -def _scope_superuser(qs, profile, user): - """leads / opportunities / invoices rule: admin = role ADMIN or superuser.""" - if is_org_admin(profile) or user.is_superuser: - return qs - return qs.filter(_own_filter(profile)).distinct() - - class GlobalSearchView(APIView): """``GET /api/search/?q=``. A handful of matches per record type.""" @@ -79,7 +58,7 @@ class GlobalSearchView(APIView): results = [] # Leads - leads = _scope_superuser(Lead.objects.filter(org=org), profile, user).filter( + leads = visible_leads_qs(profile, user).filter( Q(title__icontains=q) | Q(first_name__icontains=q) | Q(last_name__icontains=q) @@ -98,15 +77,15 @@ class GlobalSearchView(APIView): ) # Deals (Opportunity) - deals = _scope_superuser( - Opportunity.objects.filter(org=org).select_related("account"), - profile, - user, - ).filter( - Q(name__icontains=q) - | Q(description__icontains=q) - | Q(account__name__icontains=q) - )[:PER_TYPE] + deals = ( + visible_deals_qs(profile, user) + .select_related("account") + .filter( + Q(name__icontains=q) + | Q(description__icontains=q) + | Q(account__name__icontains=q) + )[:PER_TYPE] + ) for deal in deals: results.append( { @@ -120,7 +99,7 @@ class GlobalSearchView(APIView): ) # Accounts - accounts = _scope_orgadmin(Account.objects.filter(org=org), profile).filter( + accounts = visible_accounts_qs(profile, user).filter( Q(name__icontains=q) | Q(email__icontains=q) | Q(website__icontains=q) @@ -137,7 +116,7 @@ class GlobalSearchView(APIView): ) # Contacts - contacts = _scope_orgadmin(Contact.objects.filter(org=org), profile).filter( + contacts = visible_contacts_qs(profile).filter( Q(first_name__icontains=q) | Q(last_name__icontains=q) | Q(email__icontains=q) @@ -177,16 +156,16 @@ class GlobalSearchView(APIView): ) # Invoices - invoices = _scope_superuser( - Invoice.objects.filter(org=org).select_related("account"), - profile, - user, - ).filter( - Q(invoice_number__icontains=q) - | Q(invoice_title__icontains=q) - | Q(client_name__icontains=q) - | Q(account__name__icontains=q) - )[:PER_TYPE] + invoices = ( + visible_invoices_qs(profile, user) + .select_related("account") + .filter( + Q(invoice_number__icontains=q) + | Q(invoice_title__icontains=q) + | Q(client_name__icontains=q) + | Q(account__name__icontains=q) + )[:PER_TYPE] + ) for invoice in invoices: results.append( { diff --git a/backend/common/tests/test_activity_feed_scoping.py b/backend/common/tests/test_activity_feed_scoping.py new file mode 100644 index 00000000..e4ed411e --- /dev/null +++ b/backend/common/tests/test_activity_feed_scoping.py @@ -0,0 +1,197 @@ +"""D43: the recent-activity feed answers with each record's own read rule. + +Both feeds (``activities`` on ``GET /api/dashboard/`` and ``GET +/api/activities/``) returned every activity in the org to every member. An +activity row carries the record's name and what was done to it, so a member +read the names of accounts, deals and tickets hidden from them everywhere else. + +The rule now: an admin sees the org's feed; anyone else sees an activity only +while its record passes that module's read rule. An entity type with no read +rule, and an activity whose record is gone, fail closed for a member. + +``ActivityListView`` and ``ApiTodayView`` also lacked ``HasOrgContext``, so the +without-org-claim case is pinned here too. +""" + +import uuid + +import pytest +from django.db import connection +from django.test.utils import CaptureQueriesContext +from rest_framework import status +from rest_framework.test import APIClient +from rest_framework_simplejwt.tokens import RefreshToken + +from accounts.models import Account +from cases.models import Case, CaseWatcher +from common.models import Activity +from conftest import rls_org + +pytestmark = pytest.mark.django_db + +FEED = "/api/activities/" +HOME = "/api/dashboard/" + + +def _stamp(model, obj, user): + """Set ``created_by`` without the thread-local request user restamping it.""" + model.objects.filter(pk=obj.pk).update(created_by=user) + obj.refresh_from_db() + return obj + + +def _activity(org, entity_type, entity_id, name, actor=None): + return Activity.objects.create( + user=actor, + action="UPDATE", + entity_type=entity_type, + entity_id=entity_id, + entity_name=name, + org=org, + ) + + +def _feed_names(client, url=FEED): + response = client.get(url) + assert response.status_code == status.HTTP_200_OK, response.content + return {row["entity_name"] for row in response.data["activities"]} + + +@pytest.fixture +def my_account(org_a, regular_user): + return _stamp( + Account, Account.objects.create(name="Mine Co", org=org_a), regular_user + ) + + +@pytest.fixture +def hidden_account(org_a, admin_user): + return _stamp( + Account, Account.objects.create(name="Hidden Co", org=org_a), admin_user + ) + + +@pytest.fixture +def feed(org_a, org_b, admin_profile, my_account, hidden_account, profile_b): + """One activity on a visible record, one on a hidden one, one abroad.""" + _activity(org_a, "Account", my_account.id, "Mine Co", admin_profile) + _activity(org_a, "Account", hidden_account.id, "Hidden Co", admin_profile) + with rls_org(org_b): + foreign = Account.objects.create(name="Foreign Co", org=org_b) + _activity(org_b, "Account", foreign.id, "Foreign Co", profile_b) + + +@pytest.mark.parametrize("url", [FEED, HOME]) +class TestBothFeedsFollowTheReadRule: + def test_member_sees_activity_on_a_record_they_can_open( + self, url, user_client, feed + ): + assert "Mine Co" in _feed_names(user_client, url) + + def test_member_does_not_see_activity_on_a_hidden_record( + self, url, user_client, feed + ): + assert "Hidden Co" not in _feed_names(user_client, url) + + def test_admin_sees_both(self, url, admin_client, feed): + names = _feed_names(admin_client, url) + assert {"Mine Co", "Hidden Co"} <= names + + def test_another_org_never_appears(self, url, admin_client, user_client, feed): + assert "Foreign Co" not in _feed_names(admin_client, url) + assert "Foreign Co" not in _feed_names(user_client, url) + + +class TestFailClosedForMembers: + def test_activity_on_a_deleted_record_is_hidden_from_a_member( + self, org_a, user_profile, user_client, admin_client + ): + # Same author and org as a visible row; only the record is missing. + _activity(org_a, "Account", uuid.uuid4(), "Gone Co", user_profile) + assert "Gone Co" not in _feed_names(user_client) + assert "Gone Co" in _feed_names(admin_client) + + def test_entity_type_without_a_read_rule_is_hidden_from_a_member( + self, org_a, user_profile, user_client, admin_client + ): + _activity(org_a, "Team", uuid.uuid4(), "Some Team", user_profile) + assert "Some Team" not in _feed_names(user_client) + assert "Some Team" in _feed_names(admin_client) + + def test_the_rule_is_each_modules_own_a_watcher_sees_the_ticket( + self, org_a, admin_user, admin_profile, user_profile, user_client + ): + """Proves the per-type rule is the module's, not a generic owner check: + tickets admit watchers, so their activity follows.""" + case = _stamp( + Case, + Case.objects.create(name="Watched", status="New", org=org_a), + admin_user, + ) + _activity(org_a, "Case", case.id, "Watched", admin_profile) + assert "Watched" not in _feed_names(user_client) + CaseWatcher.objects.create(case=case, profile=user_profile, org=org_a) + assert "Watched" in _feed_names(user_client) + + def test_entity_type_filter_still_narrows_within_what_is_visible( + self, org_a, admin_profile, user_client, my_account, hidden_account, feed + ): + response = user_client.get(FEED, {"entity_type": "Account"}) + assert {r["entity_name"] for r in response.data["activities"]} == {"Mine Co"} + + def test_the_query_count_does_not_grow_with_the_rows( + self, org_a, admin_profile, user_client, my_account + ): + def queries(): + with CaptureQueriesContext(connection) as ctx: + assert user_client.get(FEED).status_code == status.HTTP_200_OK + return len(ctx.captured_queries) + + _activity(org_a, "Account", my_account.id, "Mine Co", admin_profile) + one = queries() + for _ in range(5): + _activity(org_a, "Account", my_account.id, "Mine Co", admin_profile) + assert queries() == one + + +class TestActivityListLimit: + @pytest.mark.parametrize("limit", ["abc", "-1", "0", ""]) + def test_an_unusable_limit_is_the_default_not_a_500( + self, limit, admin_client, feed + ): + response = admin_client.get(FEED, {"limit": limit}) + assert response.status_code == status.HTTP_200_OK + assert response.data["count"] == 2 + + def test_a_usable_limit_is_honoured(self, admin_client, feed): + response = admin_client.get(FEED, {"limit": "1"}) + assert response.data["count"] == 1 + + +def _client_without_org_claim(user): + """A JWT that authenticates but names no org, as between login and org pick.""" + client = APIClient() + token = RefreshToken.for_user(user) + client.credentials(HTTP_AUTHORIZATION=f"Bearer {token.access_token}") + return client + + +@pytest.mark.parametrize("url", [FEED, "/api/dashboard/today/", HOME]) +class TestOrgContextIsRequired: + def test_without_org_context_it_is_refused_not_crashed( + self, url, admin_user, admin_profile + ): + response = _client_without_org_claim(admin_user).get(url) + assert response.status_code == status.HTTP_403_FORBIDDEN + + def test_with_org_context_it_still_answers(self, url, admin_client): + assert admin_client.get(url).status_code == status.HTTP_200_OK + + def test_the_permission_class_is_declared(self, url): + """The middleware answers 403 first today, so the pair above passes + without the class. The class is the contract; this pins it.""" + from django.urls import resolve + + from common.permissions import HasOrgContext + + assert HasOrgContext in resolve(url).func.view_class.permission_classes diff --git a/backend/common/tests/test_can_mass_import.py b/backend/common/tests/test_can_mass_import.py new file mode 100644 index 00000000..c10fc9ba --- /dev/null +++ b/backend/common/tests/test_can_mass_import.py @@ -0,0 +1,66 @@ +"""`can_mass_import`: one gate for the contact, ticket and lead CSV importers. + +The three importers each carried their own copy of the rule, and none of them +admitted a Django superuser, while every read and write rule next to them +(accounts, contacts, deals, invoices) does. +""" + +import csv +import io + +import pytest +from django.core.files.uploadedfile import SimpleUploadedFile + +from common.permissions import can_mass_import + +PREVIEWS = [ + "/api/contacts/import/preview/", + "/api/cases/import/preview/", + "/api/leads/import/preview/", +] + + +def _upload(): + buf = io.StringIO() + writer = csv.writer(buf) + writer.writerow(["first_name", "last_name", "email", "name"]) + writer.writerow(["Ada", "Byte", "ada@example.com", "Printer on fire"]) + return SimpleUploadedFile("rows.csv", buf.getvalue().encode("utf-8"), "text/csv") + + +@pytest.mark.django_db +class TestPredicate: + def test_no_profile_is_refused(self): + assert can_mass_import(None) is False + + def test_plain_member_is_refused(self, user_profile): + assert can_mass_import(user_profile) is False + + def test_admin_is_admitted(self, admin_profile): + assert can_mass_import(admin_profile) is True + + def test_superuser_member_is_admitted(self, user_profile, regular_user): + regular_user.is_superuser = True + regular_user.save(update_fields=["is_superuser"]) + user_profile.refresh_from_db() + assert can_mass_import(user_profile) is True + + def test_sales_access_member_is_admitted(self, user_profile): + user_profile.has_sales_access = True + user_profile.save(update_fields=["has_sales_access"]) + assert can_mass_import(user_profile) is True + + +@pytest.mark.django_db +class TestEndpoints: + @pytest.mark.parametrize("url", PREVIEWS) + def test_plain_member_gets_403(self, user_client, url): + response = user_client.post(url, {"file": _upload()}, format="multipart") + assert response.status_code == 403 + + @pytest.mark.parametrize("url", PREVIEWS) + def test_superuser_member_is_let_through(self, user_client, regular_user, url): + regular_user.is_superuser = True + regular_user.save(update_fields=["is_superuser"]) + response = user_client.post(url, {"file": _upload()}, format="multipart") + assert response.status_code == 200, response.content diff --git a/backend/common/tests/test_csv_import_nul.py b/backend/common/tests/test_csv_import_nul.py index 43079671..42fa34e7 100644 --- a/backend/common/tests/test_csv_import_nul.py +++ b/backend/common/tests/test_csv_import_nul.py @@ -13,16 +13,10 @@ from contacts.services.csv_import import parse_and_validate as parse_contacts pytestmark = pytest.mark.django_db -def _parse_contacts(file_bytes, org, profile): - # The ticket parser also takes the importer, to resolve contact emails - # among the contacts they may open; the contact parser does not. - return parse_contacts(file_bytes, org) - - @pytest.mark.parametrize( "parse, csv_bytes", [ - (_parse_contacts, b"first_name,last_name,email\nAda,Love\x00lace,a@x.com\n"), + (parse_contacts, b"first_name,last_name,email\nAda,Love\x00lace,a@x.com\n"), (parse_tickets, b"name,status,priority\nBro\x00ken,New,High\n"), ], ids=["contacts", "tickets"], @@ -36,7 +30,7 @@ def test_nul_character_is_a_header_error(parse, csv_bytes, org_a, admin_profile) @pytest.mark.parametrize( "parse, csv_bytes", [ - (_parse_contacts, b"first_name,last_name,email\nAda,Lovelace,a@x.com\n"), + (parse_contacts, b"first_name,last_name,email\nAda,Lovelace,a@x.com\n"), (parse_tickets, b"name,status,priority\nBroken,New,High\n"), ], ids=["contacts", "tickets"], diff --git a/backend/common/tests/test_csv_import_tag_slugs.py b/backend/common/tests/test_csv_import_tag_slugs.py index 6d8449fc..72fc49d6 100644 --- a/backend/common/tests/test_csv_import_tag_slugs.py +++ b/backend/common/tests/test_csv_import_tag_slugs.py @@ -18,16 +18,10 @@ from contacts.services.csv_import import parse_and_validate as parse_contacts pytestmark = pytest.mark.django_db -def _parse_contacts(file_bytes, org, profile): - # The ticket parser also takes the importer, to resolve contact emails - # among the contacts they may open; the contact parser does not. - return parse_contacts(file_bytes, org) - - CONTACTS = b"first_name,last_name,email,tags\nAda,Lovelace,%s@x.com,%s\n" TICKETS = b"name,status,priority,tags\nBroken %s,New,High,%s\n" IMPORTERS = [ - (_parse_contacts, commit_contacts, CONTACTS), + (parse_contacts, commit_contacts, CONTACTS), (parse_tickets, commit_tickets, TICKETS), ] IDS = ["contacts", "tickets"] diff --git a/backend/common/tests/test_list_read_rule_parity.py b/backend/common/tests/test_list_read_rule_parity.py new file mode 100644 index 00000000..a86b698c --- /dev/null +++ b/backend/common/tests/test_list_read_rule_parity.py @@ -0,0 +1,354 @@ +"""D40: every list, search, picker and count answers with the detail read rule. + +Several paths carried their own copy of "who may see this record" and had +drifted from the detail view's, so a user could open a record their list, +search or dashboard hid, or be shown one that answered 403: + +* global search scoped accounts without the superuser clause; +* the account list restated the account rule inline (it agreed; it now calls + ``visible_accounts_qs`` so it cannot drift); +* the task list's account picker had no superuser clause; +* the dashboard counted contacts without account assignment, and counted + tasks for a superuser that the task detail view refuses them; +* the Today queue showed a superuser tickets and tasks they cannot open (it + also leaves out tickets a watcher can open, on purpose since D44: its ticket + action is a reply, which only the write rule allows); +* the ticket and task boards admitted superusers the detail views refuse, + the ticket board left watchers out, and the task board repeated a card + once per assignee on a task its caller created. + +Rules differ on purpose. Accounts, contacts, deals, leads and invoices admit a +Django superuser; tickets and tasks do not, because their detail views do not. +Each test here pins one path against the detail answer for the same caller. +""" + +import pytest +from django.utils import timezone +from rest_framework import status + +from accounts.models import Account +from cases.models import Case, CaseWatcher +from conftest import rls_org +from contacts.models import Contact +from tasks.models import Task + +pytestmark = pytest.mark.django_db + + +def _stamp(model, obj, user): + """Set ``created_by`` without the thread-local request user restamping it.""" + model.objects.filter(pk=obj.pk).update(created_by=user) + obj.refresh_from_db() + return obj + + +@pytest.fixture +def superuser(regular_user): + """The ``user_client`` caller, still a plain USER profile, made superuser.""" + regular_user.is_superuser = True + regular_user.save(update_fields=["is_superuser"]) + return regular_user + + +@pytest.fixture +def others_account(org_a, admin_user): + return _stamp( + Account, Account.objects.create(name="Zyx Others", org=org_a), admin_user + ) + + +@pytest.fixture +def foreign_account(org_b, user_b): + with rls_org(org_b): + return _stamp( + Account, Account.objects.create(name="Zyx Foreign", org=org_b), user_b + ) + + +@pytest.fixture +def others_case(org_a, admin_user): + case = Case.objects.create( + name="Zyx ticket", status="New", priority="Normal", org=org_a + ) + return _stamp(Case, case, admin_user) + + +@pytest.fixture +def foreign_case(org_b, user_b): + with rls_org(org_b): + case = Case.objects.create( + name="Zyx foreign ticket", status="New", priority="Normal", org=org_b + ) + return _stamp(Case, case, user_b) + + +def _watch(case, profile): + CaseWatcher.objects.create(case=case, profile=profile, org=case.org) + + +def _task(org, user, title, **extra): + task = Task.objects.create( + title=title, status="New", priority="Low", org=org, **extra + ) + return _stamp(Task, task, user) + + +@pytest.fixture +def others_task(org_a, admin_user): + return _task(org_a, admin_user, "Zyx task", due_date=timezone.localdate()) + + +@pytest.fixture +def foreign_task(org_b, user_b): + with rls_org(org_b): + return _task(org_b, user_b, "Zyx foreign task", due_date=timezone.localdate()) + + +# ── accounts: search, list, task picker ─────────────────────────────────── + + +def _search_ids(client, kind): + response = client.get("/api/search/", {"q": "Zyx"}) + assert response.status_code == status.HTTP_200_OK + return {r["id"] for r in response.data["results"] if r["type"] == kind} + + +def _account_list_ids(client): + response = client.get("/api/accounts/") + assert response.status_code == status.HTTP_200_OK + return {str(row["id"]) for row in response.data["active_accounts"]["open_accounts"]} + + +def _task_picker_account_ids(client): + response = client.get("/api/tasks/") + assert response.status_code == status.HTTP_200_OK, response.content + return {str(row["id"]) for row in response.data["accounts_list"]} + + +ACCOUNT_PATHS = [_search_ids, _account_list_ids, _task_picker_account_ids] + + +def _account_ids(path, client): + if path is _search_ids: + return _search_ids(client, "account") + return path(client) + + +class TestAccountsFollowTheDetailRule: + @pytest.mark.parametrize("path", ACCOUNT_PATHS) + def test_plain_member_is_not_shown_it(self, path, user_client, others_account): + response = user_client.get(f"/api/accounts/{others_account.pk}/") + assert response.status_code == status.HTTP_403_FORBIDDEN + assert str(others_account.id) not in _account_ids(path, user_client) + + @pytest.mark.parametrize("path", ACCOUNT_PATHS) + def test_superuser_is_shown_it(self, path, superuser, user_client, others_account): + response = user_client.get(f"/api/accounts/{others_account.pk}/") + assert response.status_code == status.HTTP_200_OK + assert str(others_account.id) in _account_ids(path, user_client) + + @pytest.mark.parametrize("path", ACCOUNT_PATHS) + def test_superuser_never_sees_another_orgs( + self, path, superuser, user_client, others_account, foreign_account + ): + ids = _account_ids(path, user_client) + assert str(others_account.id) in ids + assert str(foreign_account.id) not in ids + + +# ── dashboard counts ────────────────────────────────────────────────────── + + +def _home(client): + response = client.get("/api/dashboard/") + assert response.status_code == status.HTTP_200_OK + return response.data + + +class TestDashboardContactCount: + @pytest.fixture + def others_contact(self, org_a, admin_user): + return _stamp( + Contact, + Contact.objects.create(first_name="Zyx", last_name="C", org=org_a), + admin_user, + ) + + @pytest.fixture + def foreign_contact(self, org_b, user_b): + with rls_org(org_b): + return _stamp( + Contact, + Contact.objects.create(first_name="Zyx", last_name="F", org=org_b), + user_b, + ) + + def test_plain_member_does_not_count_it(self, user_client, others_contact): + assert _home(user_client)["contacts_count"] == 0 + + def test_member_assigned_to_its_account_counts_it( + self, org_a, user_profile, user_client, others_contact + ): + account = Account.objects.create(name="Via account", org=org_a) + account.assigned_to.add(user_profile) + account.contacts.add(others_contact) + # The detail view and the list already admit this member. + response = user_client.get(f"/api/contacts/{others_contact.pk}/") + assert response.status_code == status.HTTP_200_OK + assert _home(user_client)["contacts_count"] == 1 + + def test_another_orgs_contact_is_never_counted( + self, superuser, user_client, others_contact, foreign_contact + ): + assert _home(user_client)["contacts_count"] == 1 + + +class TestDashboardAccountCount: + def test_superuser_counts_it_and_a_member_does_not( + self, regular_user, user_client, others_account, foreign_account + ): + assert _home(user_client)["accounts_count"] == 0 + regular_user.is_superuser = True + regular_user.save(update_fields=["is_superuser"]) + assert _home(user_client)["accounts_count"] == 1 + + +class TestDashboardTasks: + """Tasks have no superuser clause, so the dashboard must not invent one.""" + + def test_superuser_does_not_get_a_task_they_cannot_open( + self, superuser, user_client, others_task + ): + response = user_client.get(f"/api/tasks/{others_task.pk}/") + assert response.status_code == status.HTTP_403_FORBIDDEN + data = _home(user_client) + assert str(others_task.id) not in {str(t["id"]) for t in data["tasks"]} + assert data["urgent_counts"]["tasks_due_today"] == 0 + + def test_assignee_gets_it(self, user_profile, user_client, others_task): + others_task.assigned_to.add(user_profile) + data = _home(user_client) + assert str(others_task.id) in {str(t["id"]) for t in data["tasks"]} + assert data["urgent_counts"]["tasks_due_today"] == 1 + + def test_another_orgs_task_is_never_counted( + self, user_profile, user_client, others_task, foreign_task + ): + others_task.assigned_to.add(user_profile) + data = _home(user_client) + assert str(foreign_task.id) not in {str(t["id"]) for t in data["tasks"]} + assert data["urgent_counts"]["tasks_due_today"] == 1 + + +# ── Today queue ─────────────────────────────────────────────────────────── + + +def _queue_ids(client): + response = client.get("/api/dashboard/today/") + assert response.status_code == status.HTTP_200_OK + return {row["id"] for row in response.data["queue"]} + + +class TestTodayFollowsTheDetailRule: + def test_watcher_can_open_the_ticket_but_is_not_asked_to_reply( + self, user_profile, user_client, others_case + ): + """D44: every queue row carries an action, and a ticket's is "Reply", + which the write rule refuses a watcher. The read rule still lets them + open it; the queue just does not offer them a button that 403s. The + full pair lives in ``test_today_actionable.py``.""" + _watch(others_case, user_profile) + response = user_client.get(f"/api/cases/{others_case.pk}/") + assert response.status_code == status.HTTP_200_OK + assert f"case-{others_case.id}" not in _queue_ids(user_client) + + def test_plain_member_does_not_see_it(self, user_client, others_case): + assert f"case-{others_case.id}" not in _queue_ids(user_client) + + def test_superuser_does_not_get_rows_they_cannot_open( + self, superuser, user_client, others_case, others_task + ): + assert ( + user_client.get(f"/api/cases/{others_case.pk}/").status_code + == status.HTTP_403_FORBIDDEN + ) + ids = _queue_ids(user_client) + assert f"case-{others_case.id}" not in ids + assert f"task-{others_task.id}" not in ids + + def test_another_orgs_rows_never_appear( + self, user_profile, user_client, others_case, foreign_case, foreign_task + ): + others_case.assigned_to.add(user_profile) + ids = _queue_ids(user_client) + assert f"case-{others_case.id}" in ids + assert f"case-{foreign_case.id}" not in ids + assert f"task-{foreign_task.id}" not in ids + + +# ── boards ──────────────────────────────────────────────────────────────── + + +def _board_ids(client, url, key): + response = client.get(url) + assert response.status_code == status.HTTP_200_OK, response.content + return [str(card["id"]) for col in response.data["columns"] for card in col[key]] + + +def _case_board(client): + return _board_ids(client, "/api/cases/kanban/", "cases") + + +def _task_board(client): + return _board_ids(client, "/api/tasks/kanban/", "tasks") + + +class TestCaseBoard: + def test_watcher_sees_it(self, user_profile, user_client, others_case): + _watch(others_case, user_profile) + assert str(others_case.id) in _case_board(user_client) + + def test_plain_member_does_not(self, user_client, others_case): + assert str(others_case.id) not in _case_board(user_client) + + def test_superuser_does_not_see_a_ticket_they_cannot_open( + self, superuser, user_client, others_case + ): + assert str(others_case.id) not in _case_board(user_client) + + def test_another_orgs_ticket_never_appears( + self, admin_client, others_case, foreign_case + ): + ids = _case_board(admin_client) + assert str(others_case.id) in ids + assert str(foreign_case.id) not in ids + + +class TestTaskBoard: + def test_creator_with_two_assignees_sees_one_card( + self, regular_user, user_profile, admin_profile, user_client, others_task + ): + _stamp(Task, others_task, regular_user) + others_task.assigned_to.add(user_profile, admin_profile) + assert _task_board(user_client).count(str(others_task.id)) == 1 + + def test_assignee_sees_it_once( + self, user_profile, admin_profile, user_client, others_task + ): + others_task.assigned_to.add(user_profile, admin_profile) + assert _task_board(user_client).count(str(others_task.id)) == 1 + + def test_plain_member_does_not(self, user_client, others_task): + assert str(others_task.id) not in _task_board(user_client) + + def test_superuser_does_not_see_a_task_they_cannot_open( + self, superuser, user_client, others_task + ): + assert str(others_task.id) not in _task_board(user_client) + + def test_another_orgs_task_never_appears( + self, admin_client, others_task, foreign_task + ): + ids = _task_board(admin_client) + assert str(others_task.id) in ids + assert str(foreign_task.id) not in ids diff --git a/backend/common/tests/test_packs_loader.py b/backend/common/tests/test_packs_loader.py index ed49f9f8..fe1fc17d 100644 --- a/backend/common/tests/test_packs_loader.py +++ b/backend/common/tests/test_packs_loader.py @@ -53,6 +53,34 @@ def test_win_probability_rejected_on_non_lead_stage(): validate_manifest(raw) +def _lead_stage(**stage): + return _minimal() | { + "lead_pipeline": {"name": "Sales", "stages": [{"name": "New", **stage}]} + } + + +# A lead stage's win_probability is copied onto Lead.probability by the board +# move, and both columns carry a 0-100 check constraint (D11). +@pytest.mark.parametrize("value", [150, 101, -1, "50", True, False, 50.0, None]) +def test_lead_stage_win_probability_out_of_range_or_not_an_int_is_rejected(value): + with pytest.raises( + PackValidationError, + match=r"stages\[0\]: win_probability .* must be an integer from 0 to 100", + ): + validate_manifest(_lead_stage(win_probability=value)) + + +@pytest.mark.parametrize("value", [0, 50, 100]) +def test_lead_stage_win_probability_in_range_is_accepted(value): + raw = _lead_stage(win_probability=value) + assert validate_manifest(raw) is raw + + +def test_lead_stage_without_win_probability_is_accepted(): + raw = _lead_stage() + assert validate_manifest(raw) is raw + + def test_unknown_tag_color_is_rejected(): raw = _minimal() | {"tags": [{"name": "Hot", "color": "chartreuse"}]} with pytest.raises(PackValidationError, match="chartreuse"): diff --git a/backend/common/tests/test_today_actionable.py b/backend/common/tests/test_today_actionable.py new file mode 100644 index 00000000..0f55af71 --- /dev/null +++ b/backend/common/tests/test_today_actionable.py @@ -0,0 +1,132 @@ +"""D44: every row in the Today queue offers an action its caller can take. + +D40 narrowed each Today source by its read rule. For tickets that admits +watchers, so a ticket a member only watches landed in "awaiting first reply" +with a "Reply" button, and the reply endpoint (``CaseDetailView.post``, the +write rule in ``cases/access.py``) refused it. The ticket source now uses the +write rule. Deals and tasks offer "Open", invoices "Send a reminder", and each +of those endpoints checks the same rule the source is narrowed by, so those +three were already actionable; the last test pins the invoice one. +""" + +import datetime + +import pytest +from django.utils import timezone +from rest_framework import status + +from cases.models import Case, CaseWatcher +from invoices.models import Invoice + +pytestmark = pytest.mark.django_db + +URL = "/api/dashboard/today/" + + +def _stamp(model, obj, user): + model.objects.filter(pk=obj.pk).update(created_by=user) + obj.refresh_from_db() + return obj + + +@pytest.fixture +def unanswered_case(org_a, admin_user): + """A colleague's ticket awaiting a first reply.""" + case = Case.objects.create( + name="Awaiting reply", + status="New", + priority="High", + org=org_a, + first_response_at=None, + ) + return _stamp(Case, case, admin_user) + + +def _today(client): + response = client.get(URL) + assert response.status_code == status.HTTP_200_OK, response.content + return response.data + + +def _row(data, case): + return next((r for r in data["queue"] if r["id"] == f"case-{case.id}"), None) + + +def _ticket_count(data): + return sum( + s["count"] for s in data["summary"]["sources"] if s["href"] == "/tickets" + ) + + +class TestTicketsAreOfferedOnlyToThoseWhoCanReply: + def test_watcher_only_ticket_is_not_offered( + self, user_profile, user_client, unanswered_case + ): + CaseWatcher.objects.create( + case=unanswered_case, profile=user_profile, org=unanswered_case.org + ) + data = _today(user_client) + assert _row(data, unanswered_case) is None + # The header count is of rows that want the caller, so it agrees. + assert _ticket_count(data) == 0 + assert data["summary"]["count"] == 0 + + def test_assignees_ticket_is_offered_with_reply( + self, user_profile, user_client, unanswered_case + ): + unanswered_case.assigned_to.add(user_profile) + data = _today(user_client) + row = _row(data, unanswered_case) + assert row is not None + assert row["action"] == "Reply" + assert _ticket_count(data) == 1 + + def test_the_reply_it_offers_is_accepted_and_the_one_it_hides_is_not( + self, user_profile, user_client, unanswered_case + ): + """The queue's choice is the reply endpoint's own answer.""" + CaseWatcher.objects.create( + case=unanswered_case, profile=user_profile, org=unanswered_case.org + ) + url = f"/api/cases/{unanswered_case.pk}/" + body = {"comment": "On it"} + assert ( + user_client.post(url, body, format="json").status_code + == status.HTTP_403_FORBIDDEN + ) + unanswered_case.assigned_to.add(user_profile) + assert ( + user_client.post(url, body, format="json").status_code + != status.HTTP_403_FORBIDDEN + ) + + def test_admin_is_offered_a_ticket_they_neither_opened_nor_hold( + self, admin_client, regular_user, unanswered_case + ): + _stamp(Case, unanswered_case, regular_user) + assert _row(_today(admin_client), unanswered_case) is not None + + +class TestInvoicesAreActionableByWhoeverSeesThem: + def test_assigned_invoice_is_offered_and_its_send_is_accepted( + self, org_a, admin_user, user_profile, user_client + ): + inv = Invoice.objects.create( + invoice_title="Overdue", + org=org_a, + currency="USD", + total_amount=100, + status="Sent", + due_date=timezone.localdate() - datetime.timedelta(days=3), + ) + _stamp(Invoice, inv, admin_user) + assert f"invoice-{inv.id}" not in { + r["id"] for r in _today(user_client)["queue"] + } + inv.assigned_to.add(user_profile) + row = next( + r for r in _today(user_client)["queue"] if r["id"] == f"invoice-{inv.id}" + ) + assert row["action"] == "Send a reminder" + send = user_client.post(f"/api/invoices/{inv.id}/send/", {}, format="json") + assert send.status_code == status.HTTP_200_OK diff --git a/backend/common/views/dashboard_views.py b/backend/common/views/dashboard_views.py index 34dcdef4..f098d1b7 100644 --- a/backend/common/views/dashboard_views.py +++ b/backend/common/views/dashboard_views.py @@ -9,17 +9,24 @@ from rest_framework.permissions import IsAuthenticated from rest_framework.response import Response from rest_framework.views import APIView +from accounts.access import visible_accounts_qs from accounts.models import Account +from cases.access import visible_cases_qs, writable_cases_qs from cases.models import Case from common import serializer, swagger_params from common.models import Activity from common.permissions import HasOrgContext, is_org_admin from common.utils import STAGES +from contacts.access import visible_contacts_qs from contacts.models import Contact from invoices.models import UNPAID_STATUSES, Invoice +from invoices.permissions import visible_invoices_qs +from leads.access import visible_leads_qs from leads.models import Lead +from opportunity.access import visible_deals_qs from opportunity.models import Opportunity, StageAgingConfig from opportunity.workflow import DEFAULT_STAGE_EXPECTED_DAYS, ROTTEN_MULTIPLIER +from tasks.access import visible_tasks_qs from tasks.models import Task from tasks.serializer import TaskSerializer @@ -64,23 +71,56 @@ def _fmt_date(d): return f"{d:%b} {d.day}" -def _owned_or_assigned(queryset, profile): - """Narrow a queryset to what a non-admin may see, without joining. +def _readable(queryset, visible): + """Narrow ``queryset`` to the rows in ``visible``, without joining. - Filtering straight onto the ``assigned_to`` M2M multiplies rows: a record - the member created that also carries three assignees comes back three - times, because the OR forces a LEFT JOIN and every joined row satisfies the - ``created_by`` half. That inflates ``.count()`` and, worse, ``Sum()``, so a - member's pipeline total could read three times its real value. - - Resolving the ids in a subquery keeps the outer query at one row per - record, which is what the counts and the aggregates below assume. + ``visible`` is the module's own read rule (``visible_accounts_qs`` and its + siblings), so every count and total here matches what the caller's list + shows and what the detail view opens. It is applied by id: filtering + straight onto the ``assigned_to`` M2M multiplies rows, a record carrying + three assignees comes back three times, which inflates ``.count()`` and, + worse, ``Sum()``. Resolving the ids in a subquery keeps the outer query at + one row per record, which is what the counts and the aggregates assume. """ - return queryset.filter( - pk__in=queryset.filter( - Q(assigned_to=profile) | Q(created_by=profile.user) - ).values("pk") - ) + return queryset.filter(pk__in=visible.values("pk")) + + +# The read rule for each `Activity.entity_type` a feed may name. An activity +# carries the record's name and what was done to it, so showing one is showing +# a slice of that record: it has to pass the same rule the record's own detail +# view applies. Types without an entry here (Event, Document, Team: nothing +# writes them today) have no rule to pass, so a member never sees them. +_ACTIVITY_READ_RULES = { + "Account": lambda profile, user: visible_accounts_qs(profile, user), + "Lead": lambda profile, user: visible_leads_qs(profile, user), + "Contact": lambda profile, user: visible_contacts_qs(profile), + "Opportunity": lambda profile, user: visible_deals_qs(profile, user), + "Case": lambda profile, user: visible_cases_qs(profile), + "Task": lambda profile, user: visible_tasks_qs(profile), + "Invoice": lambda profile, user: visible_invoices_qs(profile, user), +} + + +def _readable_activities(profile, user): + """The org's activities that ``profile`` may see: one query, whatever the size. + + An admin sees the whole org's feed, as before. Anyone else sees an activity + only while its record is one they can open, resolved as one id subquery per + entity type, so the cost does not grow with the number of rows returned. + That fails closed twice: an entity type with no read rule is hidden, and so + is an activity whose record has since been deleted, because no read rule + can match a row that no longer exists. + """ + qs = Activity.objects.filter(org=profile.org) + if is_org_admin(profile): + return qs + readable = Q() + for entity_type, rule in _ACTIVITY_READ_RULES.items(): + readable |= Q( + entity_type=entity_type, + entity_id__in=rule(profile, user).values("pk"), + ) + return qs.filter(readable) class ApiHomeView(APIView): @@ -113,24 +153,31 @@ class ApiHomeView(APIView): profile = request.profile today = timezone.localdate() - accounts = Account.objects.filter(is_active=True, org=org) - contacts = Contact.objects.filter(org=org) + user = request.user + + # Each one is its module's read rule, so every figure below counts the + # rows the matching list shows. They differ on purpose: contacts add + # account assignment, and tasks have no superuser clause. + accounts = _readable( + Account.objects.filter(is_active=True, org=org), + visible_accounts_qs(profile, user), + ) + contacts = _readable( + Contact.objects.filter(org=org), visible_contacts_qs(profile) + ) # Kept separate from `leads` because the conversion rate below needs # converted leads, which `leads` deliberately excludes. - all_leads = Lead.objects.filter(org=org) + all_leads = _readable( + Lead.objects.filter(org=org), visible_leads_qs(profile, user) + ) leads = all_leads.exclude(Q(status="converted") | Q(status="closed")) - opportunities = Opportunity.objects.filter(org=org) - tasks = Task.objects.filter(org=org) + opportunities = _readable( + Opportunity.objects.filter(org=org), visible_deals_qs(profile, user) + ) + tasks = _readable(Task.objects.filter(org=org), visible_tasks_qs(profile)) - is_admin = is_org_admin(profile) or request.user.is_superuser - - if not is_admin: - accounts = _owned_or_assigned(accounts, profile) - contacts = _owned_or_assigned(contacts, profile) - all_leads = _owned_or_assigned(all_leads, profile) - leads = _owned_or_assigned(leads, profile).exclude(status="closed") - opportunities = _owned_or_assigned(opportunities, profile) - tasks = _owned_or_assigned(tasks, profile) + # Decides only whether the goal summary below includes org-wide goals. + is_admin = is_org_admin(profile) or user.is_superuser # Counts only. This used to serialize every account, contact, lead and # opportunity in the org in full beside them: 372 KB of a 384 KB @@ -314,9 +361,10 @@ class ApiHomeView(APIView): for g in active_goals ] - # Include recent activities (avoid separate API call) + # Recent activities, narrowed like every figure above: a member sees + # activity only on records they can open. activities = ( - Activity.objects.filter(org=org) + _readable_activities(profile, user) .select_related("user", "user__user") .order_by("-created_at")[:10] ) @@ -340,13 +388,18 @@ class ApiTodayView(APIView): * tasks overdue or due today. Security: every query is org-scoped. The org comes from the JWT via - middleware, never the client, and a member sees only rows assigned to or - created by them, the same visibility ``ApiHomeView`` applies. Admins see the - whole org. ``HasOrgContext`` guarantees ``request.profile``/``org`` are set, - so this never dereferences a ``None`` profile. + middleware, never the client, and each source is narrowed by its own + module's read rule, as ``ApiHomeView`` does, so the queue never offers a + row the caller cannot open. Every row also carries an action, so each + source is one the caller can act on: opening a deal or a task, and sending + an invoice, need nothing beyond reading it, but replying to a ticket needs + the write rule, which leaves watchers out. A ticket someone only watches + is therefore not in the queue, nor in its count. ``HasOrgContext`` + guarantees ``request.profile``/``org`` are set, so this never dereferences + a ``None`` profile. """ - permission_classes = (IsAuthenticated,) + permission_classes = (IsAuthenticated, HasOrgContext) @extend_schema( tags=["home"], @@ -383,31 +436,39 @@ class ApiTodayView(APIView): today = timezone.localdate() yesterday = today - timedelta(days=1) week_end = today + timedelta(days=7) - is_admin = is_org_admin(profile) or request.user.is_superuser org_currency = org.default_currency or "USD" - def mine(qs): - """Restrict a queryset to rows a member may see. ``created_by`` is a - User FK (compared to ``profile.user``); ``assigned_to`` is an M2M of - Profile: the OR-over-a-join can duplicate rows, so distinct().""" - return qs.filter( - Q(assigned_to__id__in=[profile.id]) | Q(created_by=profile.user) - ).distinct() + user = request.user + # Each source is narrowed by its module's read rule, so a row here is + # one the caller can open and a count here matches the list it links + # to. Resolved once, reused by the queue, the summary and "later". + visible_deals = visible_deals_qs(profile, user) + visible_cases = visible_cases_qs(profile) + visible_invoices = visible_invoices_qs(profile, user) + visible_tasks = visible_tasks_qs(profile) # ── base, org-scoped querysets ────────────────────────────────────── - opportunities = Opportunity.objects.filter(org=org, stage__in=OPEN_STAGES) - cases = Case.objects.filter(org=org, status__in=OPEN_CASE_STATUSES) - invoices = Invoice.objects.filter( - org=org, status__in=UNPAID_STATUSES, due_date__lt=today + opportunities = _readable( + Opportunity.objects.filter(org=org, stage__in=OPEN_STAGES), visible_deals ) - tasks = Task.objects.filter( - org=org, status__in=["New", "In Progress"], due_date__lte=today + # The one source whose action needs more than reading: "Reply" is a + # write, so a ticket the caller only watches would be a dead button. + cases = _readable( + Case.objects.filter(org=org, status__in=OPEN_CASE_STATUSES), + writable_cases_qs(profile), + ) + invoices = _readable( + Invoice.objects.filter( + org=org, status__in=UNPAID_STATUSES, due_date__lt=today + ), + visible_invoices, + ) + tasks = _readable( + Task.objects.filter( + org=org, status__in=["New", "In Progress"], due_date__lte=today + ), + visible_tasks, ) - if not is_admin: - opportunities = mine(opportunities) - cases = mine(cases) - invoices = mine(invoices) - tasks = mine(tasks) # ── deal aging as DB date cutoffs (no per-row Python) ─────────────── # For each open stage: "quiet" (yellow+) once the deal has sat past its @@ -562,15 +623,16 @@ class ApiTodayView(APIView): # ── "cleared yesterday" (a morale line) ───────────────────────────── # Tasks have no completed_at, so proxy with "marked Completed and last # touched yesterday"; cases carry a real closed_on date. - cleared_tasks = Task.objects.filter( - org=org, status="Completed", updated_at__date=yesterday + cleared_tasks = _readable( + Task.objects.filter( + org=org, status="Completed", updated_at__date=yesterday + ), + visible_tasks, ) - cleared_cases = Case.objects.filter( - org=org, status="Closed", closed_on=yesterday + cleared_cases = _readable( + Case.objects.filter(org=org, status="Closed", closed_on=yesterday), + visible_cases, ) - if not is_admin: - cleared_tasks = mine(cleared_tasks) - cleared_cases = mine(cleared_cases) summary = { "count": total_urgent, @@ -583,22 +645,30 @@ class ApiTodayView(APIView): # ── "later this week" (due tomorrow … +7 days) ────────────────────── soon = Q(due_date__gt=today, due_date__lte=week_end) - later_tasks = Task.objects.filter( - org=org, status__in=["New", "In Progress"] - ).filter(soon) - later_opps = Opportunity.objects.filter( - org=org, stage__in=OPEN_STAGES, closed_on__gt=today, closed_on__lte=week_end + later_tasks = _readable( + Task.objects.filter(org=org, status__in=["New", "In Progress"]).filter( + soon + ), + visible_tasks, ) - later_invoices = Invoice.objects.filter( - org=org, - status__in=UNPAID_STATUSES, - due_date__gt=today, - due_date__lte=week_end, + later_opps = _readable( + Opportunity.objects.filter( + org=org, + stage__in=OPEN_STAGES, + closed_on__gt=today, + closed_on__lte=week_end, + ), + visible_deals, + ) + later_invoices = _readable( + Invoice.objects.filter( + org=org, + status__in=UNPAID_STATUSES, + due_date__gt=today, + due_date__lte=week_end, + ), + visible_invoices, ) - if not is_admin: - later_tasks = mine(later_tasks) - later_opps = mine(later_opps) - later_invoices = mine(later_invoices) later_rows = [] for t in later_tasks.order_by("due_date")[:10]: @@ -647,12 +717,13 @@ class ApiTodayView(APIView): class ActivityListView(APIView): - """ - Get recent activities for the organization - Returns the last 10 activities by default + """Recent activities the caller may see, newest first, 10 by default. + + Scoped the way the dashboard's feed is (`_readable_activities`): an admin + sees the org, a member sees activity on records they can open. """ - permission_classes = (IsAuthenticated,) + permission_classes = (IsAuthenticated, HasOrgContext) @extend_schema( tags=["activities"], @@ -674,18 +745,18 @@ class ActivityListView(APIView): responses={200: serializer.DashboardActivitySerializer(many=True)}, ) def get(self, request, *args, **kwargs): - if not request.profile: - return Response( - {"error": True, "errors": "Organization context required"}, - status=status.HTTP_400_BAD_REQUEST, - ) - - # Get query params - limit = min(int(request.query_params.get("limit", 10)), 50) + # A malformed or negative `limit` used to reach `int()` and the slice + # unguarded, a 500 either way. Anything unusable is the default. + try: + limit = int(request.query_params.get("limit", 10)) + except (TypeError, ValueError): + limit = 10 + if limit < 1: + limit = 10 + limit = min(limit, 50) entity_type = request.query_params.get("entity_type", None) - # Query activities for this organization - queryset = Activity.objects.filter(org=request.profile.org) + queryset = _readable_activities(request.profile, request.user) # Filter by entity type if specified if entity_type: diff --git a/backend/common/views/user_views.py b/backend/common/views/user_views.py index 8c7ebe09..9378dc30 100644 --- a/backend/common/views/user_views.py +++ b/backend/common/views/user_views.py @@ -11,7 +11,7 @@ from rest_framework.response import Response from rest_framework.views import APIView from cases.models import Case -from cases.serializer import CaseSerializer +from cases.serializer import CaseSerializer, parent_access_context from common import swagger_params from common.models import Comment, PersonalAccessToken, Profile, Teams, User from common.permissions import HasOrgContext, is_org_admin @@ -341,7 +341,9 @@ class UserDetailView(APIView): ) context["contacts"] = ContactSerializer(contacts, many=True).data cases = Case.objects.filter(assigned_to=profile_obj, org=request.profile.org) - context["cases"] = CaseSerializer(cases, many=True).data + context["cases"] = CaseSerializer( + cases, many=True, context=parent_access_context(request.profile, cases) + ).data context["assigned_data"] = assigned_data comments = Comment.objects.filter( commented_by=profile_obj, org=request.profile.org diff --git a/backend/contacts/access.py b/backend/contacts/access.py index c11e7a24..5d595dc5 100644 --- a/backend/contacts/access.py +++ b/backend/contacts/access.py @@ -37,8 +37,15 @@ def has_contact_access(profile, contact): Assignment to the account the person belongs to counts as access. Whoever owns the company owns the conversation with the people at it, and there is no reading under which that is true for viewing but false for editing. + + A Django superuser sees every contact in their own org, as they do every + account, deal and invoice. The flag is read from ``profile.user``, the + identity the request authenticated as, so a caller without a request (the + case CSV import, through `visible_contacts_qs`) gets the same answer. Like + the admin branch, it does not check the org itself: every caller fetches + ``contact`` with ``org=profile.org`` first. """ - if is_org_admin(profile): + if is_org_admin(profile) or profile.user.is_superuser: return True if profile.user_id == contact.created_by_id: return True @@ -51,7 +58,7 @@ def has_contact_access(profile, contact): def visible_contacts_qs(profile): """Contacts ``profile`` may open, the queryset form of `has_contact_access`.""" qs = Contact.objects.filter(org=profile.org) - if is_org_admin(profile): + if is_org_admin(profile) or profile.user.is_superuser: return qs my_accounts = profile.account_assigned_users.values_list("id", flat=True) return qs.filter( diff --git a/backend/contacts/import_views.py b/backend/contacts/import_views.py index 53c39121..61e21e4d 100644 --- a/backend/contacts/import_views.py +++ b/backend/contacts/import_views.py @@ -2,7 +2,8 @@ Preview reads the uploaded file and returns row-level validation results without touching the DB. Commit re-runs validation and writes inside a single -transaction. Both endpoints are gated to ADMIN or sales-access users so +transaction. Both endpoints are gated by +`common.permissions.can_mass_import` (admins, superusers, sales access) so non-privileged members cannot mass-create contacts through this surface. """ @@ -13,23 +14,12 @@ from rest_framework.permissions import IsAuthenticated from rest_framework.response import Response from rest_framework.views import APIView -from common.permissions import HasOrgContext +from common.permissions import HasOrgContext, can_mass_import from contacts.services.csv_import import commit_rows, parse_and_validate MAX_UPLOAD_BYTES = 5 * 1024 * 1024 # 5 MB; matches the UI hint -def _can_import(profile) -> bool: - """Mass-create requires admin or explicit sales-access permission.""" - if profile is None: - return False - if getattr(profile, "role", None) == "ADMIN": - return True - if getattr(profile, "is_admin", False): - return True - return bool(getattr(profile, "has_sales_access", False)) - - def _read_upload(request): """Return (file_bytes, error_response). One of the two will be None. @@ -83,7 +73,7 @@ class ContactImportPreviewView(APIView): }, ) def post(self, request, *args, **kwargs): - if not _can_import(request.profile): + if not can_mass_import(request.profile): return Response( {"error": True, "message": "Permission denied"}, status=status.HTTP_403_FORBIDDEN, @@ -91,7 +81,7 @@ class ContactImportPreviewView(APIView): file_bytes, err = _read_upload(request) if err is not None: return err - result = parse_and_validate(file_bytes, request.profile.org) + result = parse_and_validate(file_bytes, request.profile.org, request.profile) return Response(result.to_dict(), status=status.HTTP_200_OK) @@ -117,7 +107,7 @@ class ContactImportCommitView(APIView): }, ) def post(self, request, *args, **kwargs): - if not _can_import(request.profile): + if not can_mass_import(request.profile): return Response( {"error": True, "message": "Permission denied"}, status=status.HTTP_403_FORBIDDEN, diff --git a/backend/contacts/serializer.py b/backend/contacts/serializer.py index 8c56c6a2..3d2c48d4 100644 --- a/backend/contacts/serializer.py +++ b/backend/contacts/serializer.py @@ -1,6 +1,7 @@ from drf_spectacular.utils import extend_schema_field from rest_framework import serializers +from accounts.access import has_account_access from common.serializer import ( AttachmentsSerializer, OrganizationSerializer, @@ -138,9 +139,14 @@ class CreateContactSerializer(serializers.ModelSerializer): # or, worse, a check that quietly passes because there was nothing to # compare against. self.org = request_obj.profile.org if request_obj else None + self.profile = request_obj.profile if request_obj else None + self.user = request_obj.user if request_obj else None + # An id that matches no account at all reads the same as one the + # caller may not open; see `validate_account`. + self.fields["account"].error_messages["does_not_exist"] = "No such account." def validate_account(self, account): - """An account from somebody else's org is not a valid link. + """An account the caller may not open is not a valid link. `account` is a plain ModelSerializer field, so DRF resolved it against `Account.objects.all()` -- every account in the database, not the ones @@ -149,8 +155,28 @@ class CreateContactSerializer(serializers.ModelSerializer): that in a correctly configured deployment, because the lookup runs under the tenant policy; it did not stop it here, where the dev role is a superuser. The org filter is the contract either way. + + Nor is an account in this org that the caller may not open. Linking a + contact to one hands the contact to that account's assignees (see + `contacts.access.has_contact_access`), so a member could push a person + into a company they cannot see. The rule is `has_account_access`, the + one the account detail view applies, and an unknown id, another org's + account and a hidden one all get the same message, so a response never + confirms that a hidden account exists. + + The account the contact is already linked to is kept whoever saves. + The caller's form never had a choice about it, and re-sending it (the + mobile form always does) is not a new link. """ - if account is not None and (self.org is None or account.org_id != self.org.id): + if account is None: + return account + if self.instance is not None and account.id == self.instance.account_id: + return account + if ( + self.org is None + or account.org_id != self.org.id + or not has_account_access(self.profile, self.user, account) + ): raise serializers.ValidationError("No such account.") return account diff --git a/backend/contacts/services/csv_import.py b/backend/contacts/services/csv_import.py index a9ec1e70..bda942bf 100644 --- a/backend/contacts/services/csv_import.py +++ b/backend/contacts/services/csv_import.py @@ -17,9 +17,15 @@ Duplicate policy (per org): disambiguate from an existing same-named contact. Two people can legitimately share a name when other identifiers differ. +Phone and full name are matched only against contacts the importer may open +(`visible_contacts_qs`): nothing in the database forces them unique, so a +refusal that named a hidden contact would only confirm it exists. Email is +checked across the org because the constraint forces it. + All reference lookups (account, assignees, teams) are bulk-prefetched once per call (one SELECT per reference type), scoped to the caller's org so a -malicious CSV cannot reach across tenants. +malicious CSV cannot reach across tenants. `account_name` resolves only among +the accounts the importer may open, so a hidden account reads as a missing one. """ from __future__ import annotations @@ -35,10 +41,11 @@ from django.core.validators import URLValidator from django.db import IntegrityError, transaction from django.db.models.functions import Lower -from accounts.models import Account +from accounts.access import visible_accounts_qs from common.models import Profile, Tags, Teams from common.utils import COUNTRIES from common.validators import normalize_phone +from contacts.access import visible_contacts_qs from contacts.models import Contact from contacts.services.account_link import link_primary_account @@ -212,11 +219,12 @@ def _parse_bool(raw: str) -> bool | None: return None # caller treats None as "invalid input" -def parse_and_validate(file_bytes: bytes, org) -> ImportResult: +def parse_and_validate(file_bytes: bytes, org, profile) -> ImportResult: """Parse a CSV byte string and validate every row against the given org. All reference and duplicate lookups are scoped to `org` so a malicious CSV - cannot reach across tenants. + cannot reach across tenants. `profile` is the importer, whose account + access bounds `account_name`. """ text = _decode(file_bytes) if text is None: @@ -279,7 +287,7 @@ def parse_and_validate(file_bytes: bytes, org) -> ImportResult: } parsed.append((idx, record)) - ref_maps = _build_ref_maps(parsed, org) + ref_maps = _build_ref_maps(parsed, org, profile) valid: list[ValidatedRow] = [] errors: list[RowError] = [] @@ -311,7 +319,7 @@ def parse_and_validate(file_bytes: bytes, org) -> ImportResult: return ImportResult(valid=valid, errors=errors) -def _build_ref_maps(parsed: list[tuple[int, dict[str, str]]], org) -> _RefMaps: +def _build_ref_maps(parsed: list[tuple[int, dict[str, str]]], org, profile) -> _RefMaps: """Bulk-prefetch every reference value referenced anywhere in the file. One query per reference type, scoped to `org`. Keys are lowercased so @@ -348,10 +356,14 @@ def _build_ref_maps(parsed: list[tuple[int, dict[str, str]]], org) -> _RefMaps: if first and last and not email and not phone: candidate_full_names.add(f"{first.lower()}|{last.lower()}") + # Only accounts the importer may open. Resolving across the whole org told + # the importer a hidden account existed ("No account named ..." for a name + # nobody holds, a valid row for a hidden one), and linking a contact to an + # account hands it to that account's assignees (`has_contact_access`). accounts: dict[str, str] = {} if account_names: for pk, name_lower in ( - Account.objects.filter(org=org) + visible_accounts_qs(profile, profile.user) .annotate(name_lower=Lower("name")) .filter(name_lower__in=account_names) .values_list("id", "name_lower") @@ -387,13 +399,18 @@ def _build_ref_maps(parsed: list[tuple[int, dict[str, str]]], org) -> _RefMaps: .values_list("email_lower", flat=True) ) + # Phone and full name carry no DB constraint, so unlike email these two + # checks are a courtesy, and they look only at contacts the importer may + # open. Across the whole org they were an oracle: "A contact with this + # phone number already exists" for a number held only by a hidden contact + # confirmed that contact existed. existing_phones: set[str] = set() if candidate_phones: - # Phone has no DB constraint and no normalized column; we have to scan - # all contacts that have a phone in this org and normalize in Python. - # Bounded by org size, not file size, fine for any reasonable tenant. + # No normalized column, so scan the visible contacts that have a phone + # and normalize in Python. Bounded by org size, not file size, fine for + # any reasonable tenant. for raw_phone in ( - Contact.objects.filter(org=org) + visible_contacts_qs(profile) .exclude(phone__isnull=True) .exclude(phone="") .values_list("phone", flat=True) @@ -407,7 +424,7 @@ def _build_ref_maps(parsed: list[tuple[int, dict[str, str]]], org) -> _RefMaps: # Only run this query if any row has a full name but no email and no # phone. That's the only case where full-name dedup applies. for first, last in ( - Contact.objects.filter(org=org) + visible_contacts_qs(profile) .annotate( first_lower=Lower("first_name"), last_lower=Lower("last_name"), @@ -583,7 +600,11 @@ def _validate_and_build( account_id = refs.accounts.get(account_name.lower()) if account_id is None: errors.append( - RowError(idx, "account_name", f"No account named '{account_name}'") + RowError( + idx, + "account_name", + f"No account you can open is named '{account_name}'", + ) ) assigned_ids: list[str] = [] @@ -677,7 +698,7 @@ def commit_rows(file_bytes: bytes, org, profile) -> dict[str, Any]: by another request) are caught and reported as a 400-equivalent payload so the user sees an actionable message instead of a generic 500. """ - result = parse_and_validate(file_bytes, org) + result = parse_and_validate(file_bytes, org, profile) if result.header_error: return { "error": True, diff --git a/backend/contacts/tests/test_contact_access_superuser.py b/backend/contacts/tests/test_contact_access_superuser.py new file mode 100644 index 00000000..5270d5d5 --- /dev/null +++ b/backend/contacts/tests/test_contact_access_superuser.py @@ -0,0 +1,188 @@ +"""D37: the contact read rule ignored Django ``is_superuser``. + +``has_contact_access`` and ``visible_contacts_qs`` let an org admin through +and nobody else by role, while the account, deal, lead and invoice rules all +admit a superuser too. They now read ``profile.user.is_superuser``. The org +filter stays: a superuser sees every contact in their own org, never another +org's. + +The contact list, the global search and the contact pickers on the lead, +account, deal, ticket and task lists each carried an inline copy of the rule +that knew neither superusers nor account assignment. They now call +``visible_contacts_qs``, so these tests pin them too. +""" + +import pytest +from rest_framework import status + +from accounts.models import Account +from common.models import Profile +from contacts.access import has_contact_access, visible_contacts_qs +from contacts.models import Contact + +pytestmark = pytest.mark.django_db + + +@pytest.fixture +def others_contact(org_a, admin_user): + """A contact the ``user_client`` caller neither created nor is assigned to.""" + return Contact.objects.create( + first_name="Grace", last_name="Hopper", org=org_a, created_by=admin_user + ) + + +@pytest.fixture +def foreign_contact(org_b, user_b): + """A contact in the other tenant.""" + return Contact.objects.create( + first_name="Gracie", last_name="Elsewhere", org=org_b, created_by=user_b + ) + + +@pytest.fixture +def superuser(regular_user): + """The ``user_client`` caller, with a plain USER profile, made superuser.""" + regular_user.is_superuser = True + regular_user.save(update_fields=["is_superuser"]) + return regular_user + + +def _fresh(profile): + """Reload so ``profile.user`` reflects the flag as the database holds it.""" + return Profile.objects.select_related("user", "org").get(pk=profile.pk) + + +class TestPredicate: + def test_plain_member_is_refused(self, user_profile, others_contact): + assert has_contact_access(_fresh(user_profile), others_contact) is False + + def test_superuser_without_an_admin_profile_is_admitted( + self, superuser, user_profile, others_contact + ): + profile = _fresh(user_profile) + assert profile.role != "ADMIN" + assert has_contact_access(profile, others_contact) is True + + +class TestQueryset: + def test_plain_member_does_not_see_it(self, user_profile, others_contact): + assert not visible_contacts_qs(_fresh(user_profile)).filter( + pk=others_contact.pk + ) + + def test_superuser_sees_every_contact_in_their_org( + self, superuser, user_profile, others_contact + ): + assert visible_contacts_qs(_fresh(user_profile)).filter(pk=others_contact.pk) + + def test_superuser_does_not_see_another_orgs_contact( + self, superuser, user_profile, others_contact, foreign_contact + ): + ids = set( + visible_contacts_qs(_fresh(user_profile)).values_list("id", flat=True) + ) + assert others_contact.id in ids + assert foreign_contact.id not in ids + + +class TestDetail: + def test_plain_member_is_refused(self, user_client, others_contact): + response = user_client.get(f"/api/contacts/{others_contact.pk}/") + assert response.status_code == status.HTTP_403_FORBIDDEN + + def test_superuser_may_open_it(self, superuser, user_client, others_contact): + response = user_client.get(f"/api/contacts/{others_contact.pk}/") + assert response.status_code == status.HTTP_200_OK, response.content + + def test_superuser_may_not_open_another_orgs_contact( + self, superuser, user_client, foreign_contact + ): + response = user_client.get(f"/api/contacts/{foreign_contact.pk}/") + assert response.status_code == status.HTTP_404_NOT_FOUND + + +class TestList: + def _ids(self, client): + response = client.get("/api/contacts/") + assert response.status_code == status.HTTP_200_OK + return {row["id"] for row in response.data["results"]} + + def test_plain_member_does_not_see_it(self, user_client, others_contact): + assert str(others_contact.id) not in self._ids(user_client) + + def test_superuser_sees_it_but_not_another_orgs( + self, superuser, user_client, others_contact, foreign_contact + ): + ids = self._ids(user_client) + assert str(others_contact.id) in ids + assert str(foreign_contact.id) not in ids + + def test_member_sees_a_contact_at_an_account_they_are_assigned( + self, org_a, user_profile, user_client, others_contact + ): + # The detail page already opened this one; the list used to hide it. + account = Account.objects.create(name="Navy", org=org_a) + account.assigned_to.add(user_profile) + account.contacts.add(others_contact) + response = user_client.get(f"/api/contacts/{others_contact.pk}/") + assert response.status_code == status.HTTP_200_OK + assert str(others_contact.id) in self._ids(user_client) + + +class TestDelete: + def test_plain_member_is_refused(self, user_client, others_contact): + response = user_client.delete(f"/api/contacts/{others_contact.pk}/") + assert response.status_code == status.HTTP_403_FORBIDDEN + assert Contact.objects.filter(pk=others_contact.pk).exists() + + def test_superuser_may_delete_it(self, superuser, user_client, others_contact): + response = user_client.delete(f"/api/contacts/{others_contact.pk}/") + assert response.status_code == status.HTTP_200_OK, response.content + assert not Contact.objects.filter(pk=others_contact.pk).exists() + + +class TestSearch: + def _contact_ids(self, client): + response = client.get("/api/search/", {"q": "Grac"}) + assert response.status_code == status.HTTP_200_OK + return {r["id"] for r in response.data["results"] if r["type"] == "contact"} + + def test_plain_member_does_not_find_it(self, user_client, others_contact): + assert str(others_contact.id) not in self._contact_ids(user_client) + + def test_superuser_finds_it_but_not_another_orgs( + self, superuser, user_client, others_contact, foreign_contact + ): + ids = self._contact_ids(user_client) + assert str(others_contact.id) in ids + assert str(foreign_contact.id) not in ids + + +class TestPickers: + """Each list endpoint's contact catalogue offers what its save path accepts.""" + + @pytest.mark.parametrize( + "path, key", + [ + ("/api/tasks/", "contacts_list"), + ("/api/cases/", "contacts_list"), + ("/api/accounts/", "contacts"), + ("/api/leads/", "contacts"), + ("/api/opportunities/", "contacts_list"), + ], + ) + def test_superuser_is_offered_it_and_a_member_is_not( + self, path, key, regular_user, user_client, others_contact, foreign_contact + ): + def offered(): + response = user_client.get(path) + assert response.status_code == status.HTTP_200_OK, response.content + return {str(row["id"]) for row in response.data[key]} + + assert str(others_contact.id) not in offered() + + regular_user.is_superuser = True + regular_user.save(update_fields=["is_superuser"]) + ids = offered() + assert str(others_contact.id) in ids + assert str(foreign_contact.id) not in ids diff --git a/backend/contacts/tests/test_contact_account_visibility.py b/backend/contacts/tests/test_contact_account_visibility.py new file mode 100644 index 00000000..e58eafe9 --- /dev/null +++ b/backend/contacts/tests/test_contact_account_visibility.py @@ -0,0 +1,322 @@ +"""A member may link a contact only to an account they can open (D39). + +`CreateContactSerializer.validate_account` refused another org's account and +nothing else, so a member could attach a contact to any account in the org. +Linking one hands the contact to that account's assignees +(`has_contact_access`), so a member could push a person into a company they +cannot see. The contact CSV import resolved `account_name` across the whole +org, so its "No account named ..." row error also said which hidden accounts +existed. + +Both now ask `accounts.access`, as tickets do since D38: the API through +`has_account_access`, the import through `visible_accounts_qs`. Every refusal +reads the same as an account that does not exist. On edit, the account the +contact is already linked to is kept, since the caller's form never chose it. +""" + +import csv +import io +import uuid +from unittest.mock import patch + +import pytest +from django.core.files.uploadedfile import SimpleUploadedFile + +from accounts.models import Account +from conftest import rls_org +from contacts.models import Contact + +CONTACTS_URL = "/api/contacts/" +PREVIEW = "/api/contacts/import/preview/" +COMMIT = "/api/contacts/import/commit/" + +pytestmark = pytest.mark.django_db + + +def _account(org, name, created_by=None): + account = Account.objects.create(name=name, org=org) + Account.objects.filter(pk=account.pk).update(created_by=created_by) + return account + + +@pytest.fixture(autouse=True) +def _no_mail(): + with patch("contacts.views.send_email_to_assigned_user.delay"): + yield + + +@pytest.fixture +def hidden(org_a, admin_user): + return _account(org_a, "Hidden Holdings", created_by=admin_user) + + +@pytest.fixture +def assigned(org_a, admin_user, user_profile): + account = _account(org_a, "Assigned Industries", created_by=admin_user) + account.assigned_to.add(user_profile) + return account + + +@pytest.fixture +def foreign(org_b, user_b): + with rls_org(org_b): + return _account(org_b, "Other Org Ltd", created_by=user_b) + + +@pytest.fixture +def superuser(regular_user): + """The `user_client` caller, with a plain USER profile, made superuser.""" + regular_user.is_superuser = True + regular_user.save(update_fields=["is_superuser"]) + return regular_user + + +@pytest.fixture +def importer(user_profile): + user_profile.has_sales_access = True + user_profile.save(update_fields=["has_sales_access"]) + return user_profile + + +@pytest.fixture +def mine_at_hidden(org_a, regular_user, hidden): + """The member's own contact, linked by an admin to an account they cannot open.""" + contact = Contact.objects.create( + first_name="Ada", last_name="Lovelace", org=org_a, account=hidden + ) + Contact.objects.filter(pk=contact.pk).update(created_by=regular_user) + hidden.contacts.add(contact) + return contact + + +def _create(client, first_name, account_id): + return client.post( + CONTACTS_URL, + {"first_name": first_name, "last_name": "Person", "account": str(account_id)}, + format="json", + ) + + +def _refusal(response): + assert response.status_code == 400, response.content + return response.json()["errors"]["account"] + + +class TestContactApiAccountLink: + def test_member_may_link_an_account_they_are_assigned_to( + self, user_client, assigned + ): + response = _create(user_client, "Assigned", assigned.id) + assert response.status_code == 200, response.content + contact = Contact.objects.get(first_name="Assigned") + assert contact.account_id == assigned.id + assert contact in assigned.contacts.all() + + def test_member_may_link_an_account_they_created( + self, user_client, org_a, regular_user + ): + mine = _account(org_a, "Mine Co", created_by=regular_user) + response = _create(user_client, "Own", mine.id) + assert response.status_code == 200, response.content + assert Contact.objects.get(first_name="Own").account_id == mine.id + + def test_hidden_account_reads_like_an_unknown_id(self, user_client, hidden): + hidden_refusal = _refusal(_create(user_client, "Hidden", hidden.id)) + unknown_refusal = _refusal(_create(user_client, "Unknown", uuid.uuid4())) + assert hidden_refusal == unknown_refusal == ["No such account."] + assert not Contact.objects.filter(first_name__in=["Hidden", "Unknown"]) + assert not hidden.contacts.exists() + + def test_admin_may_link_any_account_in_the_org(self, admin_client, hidden): + response = _create(admin_client, "Admin", hidden.id) + assert response.status_code == 200, response.content + assert Contact.objects.get(first_name="Admin").account_id == hidden.id + + def test_superuser_may_link_any_account_in_the_org( + self, superuser, user_client, hidden + ): + response = _create(user_client, "Superuser", hidden.id) + assert response.status_code == 200, response.content + assert Contact.objects.get(first_name="Superuser").account_id == hidden.id + + @pytest.mark.parametrize("caller", ["member", "admin", "superuser"]) + def test_other_org_account_is_refused_for_everyone( + self, caller, request, user_client, admin_client, foreign + ): + if caller == "superuser": + request.getfixturevalue("superuser") + client = admin_client if caller == "admin" else user_client + assert _refusal(_create(client, "Foreign", foreign.id)) == ["No such account."] + assert not Contact.objects.filter(first_name="Foreign").exists() + + def test_patch_may_not_move_a_contact_to_a_hidden_account( + self, user_client, org_a, regular_user, hidden + ): + contact = Contact.objects.create(first_name="Mover", last_name="P", org=org_a) + Contact.objects.filter(pk=contact.pk).update(created_by=regular_user) + response = user_client.patch( + f"{CONTACTS_URL}{contact.id}/", {"account": str(hidden.id)}, format="json" + ) + assert _refusal(response) == ["No such account."] + contact.refresh_from_db() + assert contact.account_id is None + assert not hidden.contacts.exists() + + def test_put_may_not_move_a_contact_to_a_hidden_account( + self, user_client, org_a, regular_user, hidden + ): + contact = Contact.objects.create(first_name="Mover", last_name="P", org=org_a) + Contact.objects.filter(pk=contact.pk).update(created_by=regular_user) + response = user_client.put( + f"{CONTACTS_URL}{contact.id}/", + {"first_name": "Mover", "last_name": "P", "account": str(hidden.id)}, + format="json", + ) + assert _refusal(response) == ["No such account."] + contact.refresh_from_db() + assert contact.account_id is None + + def test_patch_resending_a_stored_hidden_account_keeps_it( + self, user_client, hidden, mine_at_hidden + ): + # What the mobile form does: it sends `account` on every save. + response = user_client.patch( + f"{CONTACTS_URL}{mine_at_hidden.id}/", + {"title": "Countess", "account": str(hidden.id)}, + format="json", + ) + assert response.status_code == 200, response.content + mine_at_hidden.refresh_from_db() + assert mine_at_hidden.title == "Countess" + assert mine_at_hidden.account_id == hidden.id + assert mine_at_hidden in hidden.contacts.all() + + def test_patch_without_account_keeps_a_stored_hidden_account( + self, user_client, hidden, mine_at_hidden + ): + response = user_client.patch( + f"{CONTACTS_URL}{mine_at_hidden.id}/", {"title": "Countess"}, format="json" + ) + assert response.status_code == 200, response.content + mine_at_hidden.refresh_from_db() + assert mine_at_hidden.account_id == hidden.id + + def test_put_resending_a_stored_hidden_account_keeps_it( + self, user_client, hidden, mine_at_hidden + ): + response = user_client.put( + f"{CONTACTS_URL}{mine_at_hidden.id}/", + {"first_name": "Ada", "last_name": "King", "account": str(hidden.id)}, + format="json", + ) + assert response.status_code == 200, response.content + mine_at_hidden.refresh_from_db() + assert mine_at_hidden.last_name == "King" + assert mine_at_hidden.account_id == hidden.id + + def test_picker_offers_exactly_what_the_save_path_accepts( + self, user_client, assigned, hidden + ): + # Both clients build the contact form's account picker from this list. + body = user_client.get("/api/accounts/?limit=200").json() + offered = {row["id"] for row in body["active_accounts"]["open_accounts"]} + assert str(assigned.id) in offered + assert str(hidden.id) not in offered + + +def _csv(rows): + buf = io.StringIO() + writer = csv.writer(buf) + writer.writerow(["first_name", "last_name", "account_name"]) + writer.writerows(rows) + return SimpleUploadedFile( + "contacts.csv", buf.getvalue().encode("utf-8"), content_type="text/csv" + ) + + +def _messages(body): + return {e["row"]: e["message"] for e in body["errors"]} + + +class TestContactImportAccountVisibility: + def test_hidden_and_missing_accounts_get_the_same_error( + self, user_client, importer, hidden + ): + response = user_client.post( + PREVIEW, + { + "file": _csv( + [ + ["Hidden", "Row", "Hidden Holdings"], + ["Missing", "Row", "Nobody Inc"], + ] + ) + }, + format="multipart", + ) + assert response.status_code == 200, response.content + body = response.json() + assert body["summary"]["valid"] == 0 + messages = _messages(body) + assert messages[1] == "No account you can open is named 'Hidden Holdings'" + assert messages[1].replace("Hidden Holdings", "NAME") == messages[2].replace( + "Nobody Inc", "NAME" + ) + assert {e["field"] for e in body["errors"]} == {"account_name"} + + def test_visible_account_resolves_case_insensitively( + self, user_client, importer, assigned + ): + response = user_client.post( + COMMIT, + {"file": _csv([["Imported", "Row", "assigned INDUSTRIES"]])}, + format="multipart", + ) + assert response.status_code == 200, response.content + assert response.json()["created"] == 1 + contact = Contact.objects.get(first_name="Imported") + assert contact.account_id == assigned.id + assert contact in assigned.contacts.all() + + def test_commit_refuses_a_hidden_account(self, user_client, importer, hidden): + response = user_client.post( + COMMIT, + {"file": _csv([["Sneaky", "Row", "Hidden Holdings"]])}, + format="multipart", + ) + assert response.status_code == 400, response.content + assert response.json()["created"] == 0 + assert not Contact.objects.filter(first_name="Sneaky").exists() + assert not hidden.contacts.exists() + + def test_admin_resolves_an_account_a_member_cannot(self, admin_client, hidden): + response = admin_client.post( + COMMIT, + {"file": _csv([["ByAdmin", "Row", "Hidden Holdings"]])}, + format="multipart", + ) + assert response.json()["created"] == 1, response.content + assert Contact.objects.get(first_name="ByAdmin").account_id == hidden.id + + def test_superuser_resolves_an_account_a_member_cannot( + self, superuser, user_client, importer, hidden + ): + response = user_client.post( + COMMIT, + {"file": _csv([["BySuperuser", "Row", "Hidden Holdings"]])}, + format="multipart", + ) + assert response.json()["created"] == 1, response.content + assert Contact.objects.get(first_name="BySuperuser").account_id == hidden.id + + def test_other_org_account_is_missing_even_for_an_admin( + self, admin_client, foreign + ): + response = admin_client.post( + PREVIEW, + {"file": _csv([["Abroad", "Row", "Other Org Ltd"]])}, + format="multipart", + ) + assert _messages(response.json()) == { + 1: "No account you can open is named 'Other Org Ltd'" + } diff --git a/backend/contacts/tests/test_import_duplicate_visibility.py b/backend/contacts/tests/test_import_duplicate_visibility.py new file mode 100644 index 00000000..6ba42790 --- /dev/null +++ b/backend/contacts/tests/test_import_duplicate_visibility.py @@ -0,0 +1,128 @@ +"""The contact import judges phone and name clashes among visible contacts (D48). + +`can_mass_import` admits a member with sales access. The preview checked a +row's phone and full name against every contact in the org, so such a member +got "A contact with this phone number already exists" for a number held only +by a contact they cannot open, which confirmed that contact existed. Neither +field has a DB constraint (the only one on ``Contact`` is the per-org email +index), so both checks now look only at ``visible_contacts_qs``. Email stays +org-wide, because the database refuses the duplicate either way. +""" + +import csv +import io + +import pytest +from django.core.files.uploadedfile import SimpleUploadedFile + +from contacts.models import Contact + +PREVIEW = "/api/contacts/import/preview/" +PHONE_CLASH = "A contact with this phone number already exists in your organization" +NAME_CLASH = ( + "A contact with this name already exists; add an email or phone to confirm " + "it's a different person" +) + + +def _csv(headers, rows): + buf = io.StringIO() + writer = csv.writer(buf) + writer.writerow(headers) + writer.writerows(rows) + return SimpleUploadedFile( + "contacts.csv", buf.getvalue().encode("utf-8"), content_type="text/csv" + ) + + +@pytest.fixture +def importer(user_profile): + user_profile.has_sales_access = True + user_profile.save(update_fields=["has_sales_access"]) + return user_profile + + +def _contact(org, creator, first, last, phone, email): + return Contact.objects.create( + first_name=first, + last_name=last, + phone=phone, + email=email, + org=org, + created_by=creator, + ) + + +@pytest.fixture +def hidden(org_a, admin_user): + return _contact( + org_a, admin_user, "Hidden", "Person", "+1 (202) 555-0101", "hid@x.test" + ) + + +@pytest.fixture +def mine(org_a, regular_user): + return _contact( + org_a, regular_user, "Mine", "Visible", "+1 (202) 555-0202", "mine@x.test" + ) + + +def _preview(client, headers, rows): + response = client.post(PREVIEW, {"file": _csv(headers, rows)}, format="multipart") + assert response.status_code == 200, response.content + body = response.json() + return body, [(e["field"], e["message"]) for e in body["errors"]] + + +@pytest.mark.django_db +class TestPhoneClash: + HEADERS = ["first_name", "last_name", "phone"] + + def test_hidden_contact_phone_is_not_reported(self, user_client, importer, hidden): + body, errors = _preview( + user_client, self.HEADERS, [["New", "Person", "202-555-0101"]] + ) + assert errors == [] + assert body["summary"]["valid"] == 1 + + def test_visible_contact_phone_is_reported(self, user_client, importer, mine): + body, errors = _preview( + user_client, self.HEADERS, [["New", "Person", "202-555-0202"]] + ) + assert errors == [("phone", PHONE_CLASH)] + assert body["summary"]["valid"] == 0 + + def test_admin_sees_every_phone_clash(self, admin_client, admin_profile, hidden): + _body, errors = _preview( + admin_client, self.HEADERS, [["New", "Person", "202-555-0101"]] + ) + assert errors == [("phone", PHONE_CLASH)] + + +@pytest.mark.django_db +class TestFullNameClash: + HEADERS = ["first_name", "last_name"] + + def test_hidden_contact_name_is_not_reported(self, user_client, importer, hidden): + body, errors = _preview(user_client, self.HEADERS, [["hidden", "PERSON"]]) + assert errors == [] + assert body["summary"]["valid"] == 1 + + def test_visible_contact_name_is_reported(self, user_client, importer, mine): + body, errors = _preview(user_client, self.HEADERS, [["mine", "VISIBLE"]]) + assert errors == [("first_name", NAME_CLASH)] + assert body["summary"]["valid"] == 0 + + def test_admin_sees_every_name_clash(self, admin_client, admin_profile, hidden): + _body, errors = _preview(admin_client, self.HEADERS, [["Hidden", "Person"]]) + assert errors == [("first_name", NAME_CLASH)] + + +@pytest.mark.django_db +def test_email_clash_stays_org_wide(user_client, importer, hidden): + """The per-org email index refuses the row at commit anyway, so the + preview reports it whoever holds the address.""" + _body, errors = _preview( + user_client, ["first_name", "last_name", "email"], [["A", "B", "HID@x.test"]] + ) + assert [field for field, _msg in errors] == ["email"] diff --git a/backend/contacts/views.py b/backend/contacts/views.py index 622c0e8a..c2afc75b 100644 --- a/backend/contacts/views.py +++ b/backend/contacts/views.py @@ -57,8 +57,12 @@ class ContactsListView(APIView, LimitOffsetPagination): def get_context_data(self, **kwargs): params = self.request.query_params + # The list form of `access.has_contact_access`. This filter was a + # second, inline copy of the rule that knew neither superusers nor + # account assignment, so the list hid contacts their detail page + # would open. queryset = ( - self.model.objects.filter(org=self.request.profile.org) + access.visible_contacts_qs(self.request.profile) # `-id` is a random UUID, so "the list" was in no order at all -- # a page that says "most recent first" was shuffling people. The # model's own Meta.ordering is `-created_at`; this now agrees. @@ -66,11 +70,6 @@ class ContactsListView(APIView, LimitOffsetPagination): .select_related("account") .prefetch_related("account_contacts", "assigned_to__user", "teams", "tags") ) - if not is_org_admin(self.request.profile): - queryset = queryset.filter( - Q(assigned_to__in=[self.request.profile]) - | Q(created_by=self.request.profile.user) - ).distinct() if params: if params.get("name"): @@ -641,10 +640,12 @@ class ContactDetailView(APIView): def delete(self, request, pk, format=None): self.object = self.get_object(pk) # Deliberately narrower than `assert_contact_access`: an assignee may - # work on a contact, only an admin or the person who entered it may - # destroy the record. This comparison was already the right one. + # work on a contact, only an admin, a superuser or the person who + # entered it may destroy the record. The account and deal delete + # rules let superusers through; this one alone refused them. if ( not is_org_admin(self.request.profile) + and not self.request.user.is_superuser and self.request.profile.user_id != self.object.created_by_id ): return Response( diff --git a/backend/invoices/api_views.py b/backend/invoices/api_views.py index 92e12ebb..2b89bad4 100644 --- a/backend/invoices/api_views.py +++ b/backend/invoices/api_views.py @@ -1,6 +1,7 @@ import datetime import json import logging +import uuid from datetime import timedelta from decimal import Decimal @@ -27,7 +28,7 @@ from common.serializer import ( CustomFieldDefinitionSerializer, ) from common.utils import create_attachment -from common.validators import uuid_param +from common.validators import payload_id_list, uuid_param, validate_uuid from contacts.access import visible_contacts_qs from invoices.models import ( UNPAID_STATUSES, @@ -2465,6 +2466,17 @@ class InvoiceFromTimeEntriesView(APIView): into a draft invoice (Tier 3 time-tracking). Body: ``{"account_id": "", "entry_ids": ["", ...]}``. + + Org admins and Django superusers only; anyone else gets a 403 before the + body is read. Billing time is a finance action over the whole org's + logged hours, while a member may only see their own entries + (``cases.time_views._visible_entry_qs``), so a member could otherwise + bill a colleague's time and then read it back as the invoice's creator. + + Every entry must be on a ticket whose account is ``account_id``. An entry + on a ticket with no account, or with another account, is a 400 and + nothing is written. The account and the entries are looked up inside the + caller's org, so another org's ids answer 404. """ permission_classes = (IsAuthenticated, HasOrgContext) @@ -2478,24 +2490,37 @@ class InvoiceFromTimeEntriesView(APIView): build_invoice_lines_from_entries, ) + if not is_org_admin(request.profile) and not request.user.is_superuser: + return Response( + { + "error": True, + "message": "Only admins can create invoices from time entries.", + }, + status=status.HTTP_403_FORBIDDEN, + ) + org = request.profile.org account_id = request.data.get("account_id") - entry_ids = request.data.get("entry_ids") or [] - if not account_id: return Response( {"error": True, "message": "account_id is required."}, status=status.HTTP_400_BAD_REQUEST, ) - if not isinstance(entry_ids, list) or not entry_ids: + validate_uuid(account_id, "account_id") + # Deduplicated as UUIDs, not strings, so the same id sent twice (or + # once upper-case, once lower) is one entry rather than a false 404. + entry_ids = { + uuid.UUID(str(i)) + for i in payload_id_list(request.data.get("entry_ids"), "entry_ids") + } + if not entry_ids: return Response( {"error": True, "message": "entry_ids must be a non-empty list."}, status=status.HTTP_400_BAD_REQUEST, ) - try: - account = Account.objects.get(id=account_id, org=org) - except (Account.DoesNotExist, ValueError): + account = Account.objects.filter(id=account_id, org=org).first() + if account is None: return Response( {"error": True, "message": "Account not found."}, status=status.HTTP_404_NOT_FOUND, @@ -2507,7 +2532,7 @@ class InvoiceFromTimeEntriesView(APIView): .select_related("case") .filter(id__in=entry_ids, org=org) ) - if len(entries) != len(set(str(i) for i in entry_ids)): + if len(entries) != len(entry_ids): return Response( { "error": True, @@ -2516,6 +2541,17 @@ class InvoiceFromTimeEntriesView(APIView): status=status.HTTP_404_NOT_FOUND, ) + for entry in entries: + if entry.case.account_id != account.id: + return Response( + { + "error": True, + "message": f"Time entry {entry.id} is not on a " + "ticket for this account.", + }, + status=status.HTTP_400_BAD_REQUEST, + ) + try: currency, lines = build_invoice_lines_from_entries(entries) except TimeEntryInvoicingError as exc: diff --git a/backend/invoices/tests/test_invoice_contact_access.py b/backend/invoices/tests/test_invoice_contact_access.py index 8bc58ebf..896ca145 100644 --- a/backend/invoices/tests/test_invoice_contact_access.py +++ b/backend/invoices/tests/test_invoice_contact_access.py @@ -293,6 +293,10 @@ class TestInvoiceFromDealPicksAVisibleContact: @pytest.mark.django_db class TestInvoiceFromTimeEntriesPicksAVisibleContact: + """Only admins and superusers may bill time, so a member never reaches the + contact pick here: their refusal is pinned in + ``test_invoice_from_time_entries_authz.py``.""" + URL = "/api/invoices/from-time-entries/" def _post(self, client, org, account, profile, creator): @@ -301,6 +305,7 @@ class TestInvoiceFromTimeEntriesPicksAVisibleContact: status="New", priority="Normal", org=org, + account=account, created_by=creator, ) entry = TimeEntry.objects.create( @@ -319,27 +324,6 @@ class TestInvoiceFromTimeEntriesPicksAVisibleContact: format="json", ) - def test_member_gets_the_contact_they_can_open( - self, user_client, org_a, account, mine, hidden, user_profile, regular_user - ): - _newer(hidden) - account.contacts.add(mine, hidden) - - response = self._post(user_client, org_a, account, user_profile, regular_user) - - assert response.status_code == 201, response.content - assert Invoice.objects.get().contact == mine - - def test_member_with_only_hidden_contacts_gets_none( - self, user_client, org_a, account, hidden, user_profile, regular_user - ): - account.contacts.add(hidden) - - response = self._post(user_client, org_a, account, user_profile, regular_user) - - assert response.status_code == 201, response.content - assert Invoice.objects.get().contact is None - def test_admin_gets_the_newest_contact( self, admin_client, org_a, account, mine, hidden, admin_profile, admin_user ): diff --git a/backend/invoices/tests/test_invoice_from_time_entries_authz.py b/backend/invoices/tests/test_invoice_from_time_entries_authz.py new file mode 100644 index 00000000..4e38eb13 --- /dev/null +++ b/backend/invoices/tests/test_invoice_from_time_entries_authz.py @@ -0,0 +1,238 @@ +"""Who may bill time, and which time may be billed to which account (D36). + +``POST /api/invoices/from-time-entries/`` checked only for an org, so any +member could invoice any account from any entry in the org, a colleague's +included, and then read the invoice as its creator. It also never checked +that an entry's ticket belonged to the account being billed. + +The rule now: org admins and Django superusers only, and every entry must be +on a ticket for the chosen account. ``user_client`` is a plain member (role +USER); the ``superuser`` fixture turns that same caller into a superuser +without giving the profile an admin role. +""" + +import datetime +from decimal import Decimal + +import pytest +from django.utils import timezone + +from accounts.models import Account +from cases.models import Case, TimeEntry +from invoices.models import Invoice, InvoiceLineItem + +URL = "/api/invoices/from-time-entries/" + + +@pytest.fixture +def account(org_a): + return Account.objects.create(name="Billed Co", org=org_a) + + +@pytest.fixture +def other_account(org_a): + return Account.objects.create(name="Someone Else", org=org_a) + + +@pytest.fixture +def superuser(regular_user): + regular_user.is_superuser = True + regular_user.save(update_fields=["is_superuser"]) + return regular_user + + +def _entry(org, profile, account): + case = Case.objects.create( + name="Billable work", status="New", priority="Normal", org=org, account=account + ) + return TimeEntry.objects.create( + org=org, + case=case, + profile=profile, + started_at=timezone.now() - datetime.timedelta(hours=2), + ended_at=timezone.now() - datetime.timedelta(hours=1), + billable=True, + hourly_rate=Decimal("100.00"), + currency="USD", + ) + + +def _post(client, account_id, entry_ids): + return client.post( + URL, {"account_id": account_id, "entry_ids": entry_ids}, format="json" + ) + + +def _assert_nothing_written(*entries): + assert not Invoice.objects.exists() + assert not InvoiceLineItem.objects.exists() + for entry in entries: + entry.refresh_from_db() + assert entry.invoice_id is None + + +@pytest.mark.django_db +class TestWhoMayBillTime: + def test_admin_may_bill(self, admin_client, org_a, account, admin_profile): + entry = _entry(org_a, admin_profile, account) + + response = _post(admin_client, str(account.id), [str(entry.id)]) + + assert response.status_code == 201, response.content + invoice = Invoice.objects.get(id=response.json()["invoice_id"]) + assert invoice.account == account + entry.refresh_from_db() + assert entry.invoice_id == invoice.id + + def test_admin_may_bill_a_members_time( + self, admin_client, org_a, account, user_profile + ): + entry = _entry(org_a, user_profile, account) + + response = _post(admin_client, str(account.id), [str(entry.id)]) + + assert response.status_code == 201, response.content + + def test_superuser_without_an_admin_profile_may_bill( + self, superuser, user_client, org_a, account, user_profile + ): + assert user_profile.role != "ADMIN" + entry = _entry(org_a, user_profile, account) + + response = _post(user_client, str(account.id), [str(entry.id)]) + + assert response.status_code == 201, response.content + entry.refresh_from_db() + assert entry.invoice_id is not None + + def test_member_is_refused_even_for_their_own_time( + self, user_client, org_a, account, user_profile + ): + entry = _entry(org_a, user_profile, account) + + response = _post(user_client, str(account.id), [str(entry.id)]) + + assert response.status_code == 403, response.content + _assert_nothing_written(entry) + + def test_member_is_refused_a_colleagues_time( + self, user_client, org_a, account, admin_profile + ): + entry = _entry(org_a, admin_profile, account) + + response = _post(user_client, str(account.id), [str(entry.id)]) + + assert response.status_code == 403, response.content + _assert_nothing_written(entry) + + def test_member_is_refused_before_the_body_is_read(self, user_client): + """A 400 here would tell a member which ids parse, so the role check + comes first.""" + response = _post(user_client, "not-a-uuid", ["nope"]) + + assert response.status_code == 403, response.content + + +@pytest.mark.django_db +class TestEntriesMustBeOnTheAccountsTickets: + def test_entry_on_another_accounts_ticket_is_refused( + self, admin_client, org_a, account, other_account, admin_profile + ): + entry = _entry(org_a, admin_profile, other_account) + + response = _post(admin_client, str(account.id), [str(entry.id)]) + + assert response.status_code == 400, response.content + assert "not on a ticket for this account" in response.json()["message"] + _assert_nothing_written(entry) + + def test_entry_on_a_ticket_with_no_account_is_refused( + self, admin_client, org_a, account, admin_profile + ): + entry = _entry(org_a, admin_profile, None) + + response = _post(admin_client, str(account.id), [str(entry.id)]) + + assert response.status_code == 400, response.content + _assert_nothing_written(entry) + + def test_one_foreign_entry_refuses_the_whole_batch( + self, admin_client, org_a, account, other_account, admin_profile + ): + good = _entry(org_a, admin_profile, account) + foreign = _entry(org_a, admin_profile, other_account) + + response = _post(admin_client, str(account.id), [str(good.id), str(foreign.id)]) + + assert response.status_code == 400, response.content + _assert_nothing_written(good, foreign) + + +@pytest.mark.django_db +class TestMalformedIds: + @pytest.mark.parametrize("account_id", ["not-a-uuid", 123, {"id": "x"}]) + def test_malformed_account_id_is_a_400( + self, admin_client, org_a, account, admin_profile, account_id + ): + entry = _entry(org_a, admin_profile, account) + + response = _post(admin_client, account_id, [str(entry.id)]) + + assert response.status_code == 400, response.content + _assert_nothing_written(entry) + + @pytest.mark.parametrize( + "entry_ids", [["not-a-uuid"], [123], [["nested"]], {"id": "x"}, 7] + ) + def test_malformed_entry_ids_are_a_400(self, admin_client, account, entry_ids): + response = _post(admin_client, str(account.id), entry_ids) + + assert response.status_code == 400, response.content + assert not Invoice.objects.exists() + + @pytest.mark.parametrize("entry_ids", [[], None, ""]) + def test_missing_entry_ids_are_a_400(self, admin_client, account, entry_ids): + response = _post(admin_client, str(account.id), entry_ids) + + assert response.status_code == 400, response.content + + def test_missing_account_id_is_a_400(self, admin_client): + response = _post(admin_client, None, ["00000000-0000-0000-0000-000000000000"]) + + assert response.status_code == 400, response.content + + def test_a_repeated_id_is_billed_once( + self, admin_client, org_a, account, admin_profile + ): + entry = _entry(org_a, admin_profile, account) + ids = [str(entry.id), str(entry.id), str(entry.id).upper()] + + response = _post(admin_client, str(account.id), ids) + + assert response.status_code == 201, response.content + assert response.json()["line_count"] == 1 + + +@pytest.mark.django_db +class TestAnotherOrgStaysInvisible: + def test_another_orgs_entry_is_a_404( + self, admin_client, org_a, org_b, account, profile_b + ): + other_account = Account.objects.create(name="Other org", org=org_b) + entry = _entry(org_b, profile_b, other_account) + + response = _post(admin_client, str(account.id), [str(entry.id)]) + + assert response.status_code == 404, response.content + _assert_nothing_written(entry) + + def test_another_orgs_account_is_a_404( + self, admin_client, org_a, org_b, account, admin_profile + ): + other_account = Account.objects.create(name="Other org", org=org_b) + entry = _entry(org_a, admin_profile, account) + + response = _post(admin_client, str(other_account.id), [str(entry.id)]) + + assert response.status_code == 404, response.content + _assert_nothing_written(entry) diff --git a/backend/leads/migrations/0017_leadstage_win_probability_range.py b/backend/leads/migrations/0017_leadstage_win_probability_range.py new file mode 100644 index 00000000..1953addd --- /dev/null +++ b/backend/leads/migrations/0017_leadstage_win_probability_range.py @@ -0,0 +1,74 @@ +"""Bound `LeadStage.win_probability` to 0-100 at the database (tracker D11). + +The board move copies a stage's win probability onto `Lead.probability`, which +has the `lead_probability_range` check. A stage stored at 150 or -5 therefore +turned every later move into it into an IntegrityError 500. The API now refuses +such a value, and this adds the matching check so no other writer can store one. + +CLAMP FIRST + +A row already out of range would make `AddConstraint` fail, since Postgres +validates every existing row when the check is added. So the data step runs +first and clamps: below 0 becomes 0, above 100 becomes 100. Clamping keeps the +stage and its nearest meaningful value; nothing is deleted. + +THE ORG LOOP IS NOT DECORATION + +`lead_stage` is in ORG_SCOPED_TABLES, so it has `FORCE ROW LEVEL SECURITY` and +an unscoped update during a migration (where `app.current_org` is empty) +matches no row in production. The loop sets the context per org, the shape +`common/0042_recompute_tag_slugs` documents. Constraint validation is not +subject to RLS, which is exactly why every org has to be clamped. + +REVERSE + +The constraint is dropped. The clamp is a no-op: the original out-of-range +values were the defect, and nothing needs them back. +""" + +from django.db import migrations, models + + +def clamp_win_probability(apps, schema_editor): + connection = schema_editor.connection + # RLS and `set_config` are Postgres-only; on SQLite the loop still runs + # and only the context call is skipped. + is_postgres = connection.vendor == "postgresql" + + Org = apps.get_model("common", "Org") + LeadStage = apps.get_model("leads", "LeadStage") + + def set_context(value): + if not is_postgres: + return + with connection.cursor() as cursor: + cursor.execute("SELECT set_config('app.current_org', %s, false)", [value]) + + try: + for org_id in Org.objects.values_list("id", flat=True).iterator(): + set_context(str(org_id)) + stages = LeadStage.objects.filter(org_id=org_id) + stages.filter(win_probability__lt=0).update(win_probability=0) + stages.filter(win_probability__gt=100).update(win_probability=100) + finally: + set_context("") + + +class Migration(migrations.Migration): + dependencies = [ + ("common", "0042_recompute_tag_slugs"), + ("leads", "0016_alter_leadpipeline_is_default"), + ] + + operations = [ + migrations.RunPython(clamp_win_probability, migrations.RunPython.noop), + migrations.AddConstraint( + model_name="leadstage", + constraint=models.CheckConstraint( + condition=models.Q( + ("win_probability__gte", 0), ("win_probability__lte", 100) + ), + name="lead_stage_win_probability_range", + ), + ), + ] diff --git a/backend/leads/models.py b/backend/leads/models.py index b0d33d8a..6df6f102 100644 --- a/backend/leads/models.py +++ b/backend/leads/models.py @@ -325,6 +325,14 @@ class LeadStage(BaseModel): models.Index(fields=["org", "order"]), models.Index(fields=["pipeline", "order"]), ] + constraints = [ + # Copied onto Lead.probability by the board move, so it has to fit + # lead_probability_range or every move into this stage is a 500. + models.CheckConstraint( + condition=Q(win_probability__gte=0) & Q(win_probability__lte=100), + name="lead_stage_win_probability_range", + ), + ] def __str__(self): return f"{self.pipeline.name} - {self.name}" diff --git a/backend/leads/tests/test_stage_win_probability_range.py b/backend/leads/tests/test_stage_win_probability_range.py new file mode 100644 index 00000000..dda525a0 --- /dev/null +++ b/backend/leads/tests/test_stage_win_probability_range.py @@ -0,0 +1,189 @@ +"""`LeadStage.win_probability` is bounded to 0-100 at the database (D11). + +The board move copies it onto `Lead.probability`, whose `lead_probability_range` +check made an out-of-range stage a 500 on every move into it. The serializer +bound is pinned in `test_leads_kanban.py::TestLeadStageWinProbabilityBounds`; +this file pins the check constraint and `leads/0017`'s clamp. +""" + +import importlib +from contextlib import contextmanager +from types import SimpleNamespace + +import pytest +from django.apps import apps as global_apps +from django.db import IntegrityError, connection, transaction + +from leads.models import Lead, LeadPipeline, LeadStage + +CONSTRAINT = "lead_stage_win_probability_range" + + +def _set_rls(org): + if connection.vendor != "postgresql": + return + with connection.cursor() as cursor: + cursor.execute("SELECT set_config('app.current_org', %s, false)", [str(org.id)]) + + +def _stage(org, created_by, name, win_probability=0): + pipeline, _ = LeadPipeline.objects.get_or_create( + name="Sales", org=org, defaults={"created_by": created_by} + ) + return LeadStage.objects.create( + pipeline=pipeline, + name=name, + org=org, + win_probability=win_probability, + ) + + +@contextmanager +def _check_suspended(): + """Let a test store a row as production may hold one from before the check. + + SQLite cannot drop a constraint inside the test transaction, but it can be + told to skip CHECKs. Postgres drops it; the test transaction's rollback + restores it, and re-adding it here also proves the rows now satisfy it. + """ + with connection.cursor() as cursor: + if connection.vendor == "sqlite": + cursor.execute("PRAGMA ignore_check_constraints = ON") + try: + yield + finally: + cursor.execute("PRAGMA ignore_check_constraints = OFF") + else: + cursor.execute(f"ALTER TABLE lead_stage DROP CONSTRAINT {CONSTRAINT}") + yield + # Rows inserted and updated in this transaction leave deferred FK + # checks queued, and Postgres refuses ALTER TABLE while they are. + cursor.execute("SET CONSTRAINTS ALL IMMEDIATE") + cursor.execute( + f"ALTER TABLE lead_stage ADD CONSTRAINT {CONSTRAINT} " + "CHECK (win_probability >= 0 AND win_probability <= 100)" + ) + + +@pytest.mark.django_db +class TestConstraint: + @pytest.mark.parametrize("value", [-1, 101, 150]) + def test_out_of_range_is_refused_by_the_database(self, admin_user, org_a, value): + _set_rls(org_a) + stage = _stage(org_a, admin_user, "New") + + # .update() skips every Python-side check, so only the DB can refuse it. + with pytest.raises(IntegrityError), transaction.atomic(): + LeadStage.objects.filter(pk=stage.pk).update(win_probability=value) + + stage.refresh_from_db() + assert stage.win_probability == 0 + + @pytest.mark.parametrize("value", [0, 100]) + def test_the_bounds_save(self, admin_user, org_a, value): + _set_rls(org_a) + stage = _stage(org_a, admin_user, "Edge", win_probability=value) + + stage.refresh_from_db() + assert stage.win_probability == value + + +@pytest.mark.django_db +class TestClampMigration: + """`leads/0017`'s data step, called directly as `common/tests/test_tag_slugs.py` + calls `common/0042`. The live models stand in for the historical ones, + which is sound while `LeadStage` has the same fields as at 0017.""" + + def _clamp(self): + module = importlib.import_module( + "leads.migrations.0017_leadstage_win_probability_range" + ) + module.clamp_win_probability( + global_apps, SimpleNamespace(connection=connection) + ) + + def test_out_of_range_rows_are_clamped_in_every_org(self, admin_user, org_a, org_b): + with _check_suspended(): + _set_rls(org_a) + low = _stage(org_a, admin_user, "Low", -5) + high = _stage(org_a, admin_user, "High", 150) + zero = _stage(org_a, admin_user, "Zero", 0) + mid = _stage(org_a, admin_user, "Mid", 50) + hundred = _stage(org_a, admin_user, "Hundred", 100) + _set_rls(org_b) + other = _stage(org_b, admin_user, "Other org", 150) + + self._clamp() + + got = {} + for org, stage in [ + (org_a, low), + (org_a, high), + (org_a, zero), + (org_a, mid), + (org_a, hundred), + (org_b, other), + ]: + _set_rls(org) + got[stage.name] = LeadStage.objects.get(pk=stage.pk).win_probability + assert got == { + "Low": 0, + "High": 100, + "Zero": 0, + "Mid": 50, + "Hundred": 100, + "Other org": 100, + } + + def test_a_move_into_a_clamped_stage_succeeds( + self, admin_client, admin_user, org_a + ): + with _check_suspended(): + _set_rls(org_a) + stage = _stage(org_a, admin_user, "Was 150", 150) + self._clamp() + _set_rls(org_a) + lead = Lead.objects.create( + first_name="Board", + last_name="Lead", + email="clamped@example.com", + status="assigned", + created_by=admin_user, + org=org_a, + ) + + response = admin_client.patch( + f"/api/leads/{lead.id}/move/", {"stage_id": str(stage.id)}, format="json" + ) + + assert response.status_code == 200 + lead.refresh_from_db() + assert lead.stage_id == stage.id + assert lead.probability == 100 + + +@pytest.mark.django_db +class TestMoveAtTheBounds: + @pytest.mark.parametrize("value", [0, 1, 99, 100]) + def test_a_move_into_any_storable_stage_succeeds( + self, admin_client, admin_user, org_a, value + ): + _set_rls(org_a) + stage = _stage(org_a, admin_user, "Target", value) + lead = Lead.objects.create( + first_name="Board", + last_name="Lead", + email="bounds@example.com", + status="assigned", + created_by=admin_user, + org=org_a, + ) + + response = admin_client.patch( + f"/api/leads/{lead.id}/move/", {"stage_id": str(stage.id)}, format="json" + ) + + assert response.status_code == 200 + lead.refresh_from_db() + assert lead.stage_id == stage.id + assert lead.probability == value diff --git a/backend/leads/views/import_views.py b/backend/leads/views/import_views.py index 34defcc8..c801a1a8 100644 --- a/backend/leads/views/import_views.py +++ b/backend/leads/views/import_views.py @@ -3,8 +3,9 @@ single-step `upload/` alias over the same commit path. The same contract as `contacts.import_views` and `cases.import_views`, which is what the web import drawers speak. All three endpoints are gated by -`_can_import`, so a member without admin or sales access cannot mass-create -leads here even though they can create them one at a time. +`common.permissions.can_mass_import`, so a member without admin or sales +access cannot mass-create leads here even though they can create them one at +a time. """ from drf_spectacular.utils import extend_schema, inline_serializer @@ -14,26 +15,12 @@ from rest_framework.permissions import IsAuthenticated from rest_framework.response import Response from rest_framework.views import APIView -from common.permissions import HasOrgContext +from common.permissions import HasOrgContext, can_mass_import from leads.csv_import import commit_rows, parse_and_validate MAX_UPLOAD_BYTES = 5 * 1024 * 1024 # 5 MB; matches the UI hint -def _can_import(profile) -> bool: - """Mass-create requires admin or explicit sales-access permission. - - Same rule as `contacts.import_views._can_import` and its cases twin. - """ - if profile is None: - return False - if getattr(profile, "role", None) == "ADMIN": - return True - if getattr(profile, "is_admin", False): - return True - return bool(getattr(profile, "has_sales_access", False)) - - def _read_upload(request, field="file", csv_name=True): """Return (file_bytes, problem); exactly one is None. @@ -100,7 +87,7 @@ class LeadImportPreviewView(APIView): }, ) def post(self, request, *args, **kwargs): - if not _can_import(request.profile): + if not can_mass_import(request.profile): return _forbidden() file_bytes, problem = _read_upload(request) if problem: @@ -131,7 +118,7 @@ class LeadImportCommitView(APIView): }, ) def post(self, request, *args, **kwargs): - if not _can_import(request.profile): + if not can_mass_import(request.profile): return _forbidden() file_bytes, problem = _read_upload(request) if problem: @@ -182,7 +169,7 @@ class LeadUploadView(APIView): }, ) def post(self, request, *args, **kwargs): - if not _can_import(request.profile): + if not can_mass_import(request.profile): return Response( {"error": True, "errors": "Admin access required"}, status=status.HTTP_403_FORBIDDEN, diff --git a/backend/leads/views/lead_views.py b/backend/leads/views/lead_views.py index 75358bd7..ce58c630 100644 --- a/backend/leads/views/lead_views.py +++ b/backend/leads/views/lead_views.py @@ -46,8 +46,7 @@ from common.validators import ( payload_id_list, uuid_list_param, ) -from contacts.access import replace_visible_contacts -from contacts.models import Contact +from contacts.access import replace_visible_contacts, visible_contacts_qs from leads import access, swagger_params from leads.models import Lead from leads.serializer import ( @@ -237,20 +236,12 @@ class LeadListView(APIView, LimitOffsetPagination): "close_leads": close_leads, "offset": offset, } - # Narrowed for a non-admin the same way the lead queryset above is. - # This catalogue feeds the lead form's contact picker, and org scope - # alone would let a member read back the first name of every contact - # in the org, including ones `/api/contacts/` refuses them. - contact_qs = Contact.objects.filter(org=self.request.profile.org) - if ( - not is_org_admin(self.request.profile) - and not self.request.user.is_superuser - ): - contact_qs = contact_qs.filter( - Q(assigned_to__in=[self.request.profile]) - | Q(created_by=self.request.profile.user) - ).distinct() - contacts = contact_qs.values("id", "first_name") + # The contact read rule itself. This catalogue feeds the lead form's + # contact picker, and org scope alone would let a member read back the + # first name of every contact in the org, including ones + # `/api/contacts/` refuses them. It is also exactly what + # `replace_visible_contacts` accepts on save. + contacts = visible_contacts_qs(self.request.profile).values("id", "first_name") context["contacts"] = contacts context["status"] = LEAD_STATUS diff --git a/backend/opportunity/views/opportunity_views.py b/backend/opportunity/views/opportunity_views.py index 0f7f0c4b..4e9c178e 100644 --- a/backend/opportunity/views/opportunity_views.py +++ b/backend/opportunity/views/opportunity_views.py @@ -47,7 +47,6 @@ from common.validators import ( uuid_param, ) from contacts.access import replace_visible_contacts, visible_contacts_qs -from contacts.models import Contact from contacts.serializer import ContactPickerSerializer from opportunity import access, swagger_params from opportunity.models import Opportunity, StageAgingConfig @@ -151,7 +150,8 @@ class OpportunityListView(APIView, LimitOffsetPagination): "-id" ) accounts = Account.objects.filter(org=self.request.profile.org) - contacts = Contact.objects.filter(org=self.request.profile.org) + # The contact read rule itself, which is what the save path accepts. + contacts = visible_contacts_qs(self.request.profile) if ( not is_org_admin(self.request.profile) and not self.request.user.is_superuser @@ -164,10 +164,6 @@ class OpportunityListView(APIView, LimitOffsetPagination): Q(created_by=self.request.profile.user) | Q(assigned_to=self.request.profile) ).distinct() - contacts = contacts.filter( - Q(created_by=self.request.profile.user) - | Q(assigned_to=self.request.profile) - ).distinct() if params: if params.get("name"): diff --git a/backend/tasks/serializer.py b/backend/tasks/serializer.py index 5efe8745..17a7ad2d 100644 --- a/backend/tasks/serializer.py +++ b/backend/tasks/serializer.py @@ -1,3 +1,4 @@ +from django.db.models import Count, Q from drf_spectacular.utils import extend_schema_field from rest_framework import serializers @@ -10,6 +11,7 @@ from common.serializer import ( UserSerializer, ) from contacts.serializer import ContactLinkSerializer +from tasks.access import visible_tasks_qs from tasks.models import ( Board, BoardColumn, @@ -427,8 +429,20 @@ class TaskCreateSwaggerSerializer(serializers.ModelSerializer): # ============================================================================ +def _visible_task_count(serializer, **lookup): + """How many tasks matching ``lookup`` the requester may open. + + ``task_count`` once counted every task in the org, so a member read a + count that included tasks they cannot open. It follows ``visible_tasks_qs`` + now, the rule the board and the list use. The request has to be in the + serializer context: a missing one is a KeyError, never an unscoped count. + """ + profile = serializer.context["request"].profile + return visible_tasks_qs(profile).filter(**lookup).count() + + class TaskStageSerializer(serializers.ModelSerializer): - """Serializer for task stages.""" + """Serializer for task stages. Needs ``request`` in its context.""" task_count = serializers.SerializerMethodField() @@ -450,11 +464,28 @@ class TaskStageSerializer(serializers.ModelSerializer): @extend_schema_field(int) def get_task_count(self, obj): - return obj.tasks.count() + return _visible_task_count(self, stage=obj) + + def validate_maps_to_status(self, value): + """Only a real task status, or nothing. + + The column is free text, and a stage mapped to anything else is one no + task can move into: `Task.save()` runs `full_clean()` and refuses the + status the move would set. + """ + if value in (None, ""): + return value + allowed = [choice for choice, _ in Task.STATUS_CHOICES] + if value not in allowed: + raise serializers.ValidationError( + f"maps_to_status must be one of {', '.join(allowed)}, or empty." + ) + return value class TaskPipelineSerializer(serializers.ModelSerializer): - """Serializer for task pipelines with nested stages.""" + """Serializer for task pipelines with nested stages. Needs ``request`` in + its context.""" stages = TaskStageSerializer(many=True, read_only=True) stage_count = serializers.SerializerMethodField() @@ -482,11 +513,17 @@ class TaskPipelineSerializer(serializers.ModelSerializer): @extend_schema_field(int) def get_task_count(self, obj): - return Task.objects.filter(stage__pipeline=obj).count() + return _visible_task_count(self, stage__pipeline=obj) class TaskPipelineListSerializer(serializers.ModelSerializer): - """Simplified pipeline serializer for lists.""" + """Simplified pipeline serializer for lists. + + Both counts are read from annotations, so every queryset serialized here + must come through ``with_counts``: one query for the whole list, however + many pipelines. A pipeline without them raises AttributeError, which is + louder than a count that silently ignores who is asking. + """ stage_count = serializers.SerializerMethodField() task_count = serializers.SerializerMethodField() @@ -504,13 +541,25 @@ class TaskPipelineListSerializer(serializers.ModelSerializer): "created_at", ] + @staticmethod + def with_counts(pipelines, profile): + # The task join repeats a stage once per task in it, hence distinct on + # the stage count. A task sits in one stage, so it is counted once. + return pipelines.annotate( + stage_count=Count("stages", distinct=True), + task_count=Count( + "stages__tasks", + filter=Q(stages__tasks__in=visible_tasks_qs(profile).values("pk")), + ), + ) + @extend_schema_field(int) def get_stage_count(self, obj): - return obj.stages.count() + return obj.stage_count @extend_schema_field(int) def get_task_count(self, obj): - return Task.objects.filter(stage__pipeline=obj).count() + return obj.task_count class RelatedEntitySerializer(serializers.Serializer): diff --git a/backend/tasks/tests/test_board_write_verbs_hide_like_get.py b/backend/tasks/tests/test_board_write_verbs_hide_like_get.py new file mode 100644 index 00000000..94a381a5 --- /dev/null +++ b/backend/tasks/tests/test_board_write_verbs_hide_like_get.py @@ -0,0 +1,163 @@ +"""Board write verbs answer a stranger the way GET does (D46). + +Board GETs answered a non-member 404, but column create, card create, card +update and card delete answered 403, so anyone in the org could tell which +board and card ids exist. A 404 with a different body from a missing id's +told them the same thing, so the answers are compared whole. +""" + +import uuid + +import pytest + +from tasks.models import Board, BoardColumn, BoardMember, BoardTask + + +@pytest.fixture +def board(admin_profile, admin_user, org_a): + board = Board.objects.create( + name="Private", owner=admin_profile, org=org_a, created_by=admin_user + ) + BoardMember.objects.create(board=board, profile=admin_profile, role="owner") + return board + + +@pytest.fixture +def column(board, org_a): + return BoardColumn.objects.create(board=board, name="Col", order=1, org=org_a) + + +@pytest.fixture +def card(column, org_a, admin_user): + return BoardTask.objects.create( + column=column, title="Card", priority="low", org=org_a, created_by=admin_user + ) + + +def _join(board, profile, role="member"): + BoardMember.objects.create(board=board, profile=profile, role=role) + + +class TestStrangerGets404: + def test_column_create(self, user_client, board): + response = user_client.post( + f"/api/boards/{board.id}/columns/", {"name": "New"}, format="json" + ) + assert response.status_code == 404 + assert board.columns.count() == 0 + + def test_card_create(self, user_client, column): + response = user_client.post( + f"/api/boards/columns/{column.id}/tasks/", + {"title": "New", "priority": "low"}, + format="json", + ) + assert response.status_code == 404 + assert column.tasks.count() == 0 + + def test_card_update(self, user_client, card): + response = user_client.put( + f"/api/boards/tasks/{card.id}/", {"title": "Changed"}, format="json" + ) + assert response.status_code == 404 + card.refresh_from_db() + assert card.title == "Card" + + def test_card_delete(self, user_client, card): + response = user_client.delete(f"/api/boards/tasks/{card.id}/") + assert response.status_code == 404 + assert BoardTask.objects.filter(id=card.id).exists() + + +class TestMembersStillAllowed: + def test_member_updates_and_deletes_a_card(self, user_client, user_profile, card): + _join(card.column.board, user_profile) + response = user_client.put( + f"/api/boards/tasks/{card.id}/", {"title": "Changed"}, format="json" + ) + assert response.status_code == 200, response.content + assert user_client.delete(f"/api/boards/tasks/{card.id}/").status_code == 204 + + def test_member_creates_a_card(self, user_client, user_profile, column): + _join(column.board, user_profile) + response = user_client.post( + f"/api/boards/columns/{column.id}/tasks/", + {"title": "New", "priority": "low"}, + format="json", + ) + assert response.status_code == 201, response.content + + def test_plain_member_still_gets_403_on_column_create( + self, user_client, user_profile, board + ): + """A member can see the board, so a refusal here reveals nothing.""" + _join(board, user_profile) + response = user_client.post( + f"/api/boards/{board.id}/columns/", {"name": "New"}, format="json" + ) + assert response.status_code == 403 + + def test_board_admin_creates_a_column(self, user_client, user_profile, board): + _join(board, user_profile, role="admin") + response = user_client.post( + f"/api/boards/{board.id}/columns/", {"name": "New"}, format="json" + ) + assert response.status_code == 201, response.content + + +def _calls(board, column, card): + """(method, url for the real id, url for a missing id, body) per verb.""" + missing = uuid.uuid4() + return [ + ("get", f"/api/boards/{board.id}/", f"/api/boards/{missing}/", None), + ("put", f"/api/boards/{board.id}/", f"/api/boards/{missing}/", {"name": "x"}), + ("delete", f"/api/boards/{board.id}/", f"/api/boards/{missing}/", None), + ( + "get", + f"/api/boards/{board.id}/columns/", + f"/api/boards/{missing}/columns/", + None, + ), + ( + "post", + f"/api/boards/{board.id}/columns/", + f"/api/boards/{missing}/columns/", + {"name": "x"}, + ), + ( + "get", + f"/api/boards/columns/{column.id}/tasks/", + f"/api/boards/columns/{missing}/tasks/", + None, + ), + ( + "post", + f"/api/boards/columns/{column.id}/tasks/", + f"/api/boards/columns/{missing}/tasks/", + {"title": "x", "priority": "low"}, + ), + ( + "put", + f"/api/boards/tasks/{card.id}/", + f"/api/boards/tasks/{missing}/", + {"title": "x"}, + ), + ( + "delete", + f"/api/boards/tasks/{card.id}/", + f"/api/boards/tasks/{missing}/", + None, + ), + ] + + +def test_hidden_and_missing_ids_get_the_same_answer(user_client, board, column, card): + for method, real, missing, body in _calls(board, column, card): + call = getattr(user_client, method) + kwargs = {"format": "json"} if body is not None else {} + hidden = call(real, body, **kwargs) if body is not None else call(real) + absent = call(missing, body, **kwargs) if body is not None else call(missing) + assert hidden.status_code == absent.status_code == 404, (method, real) + assert hidden.json() == absent.json(), (method, real) + assert Board.objects.filter(id=board.id).exists() + assert BoardTask.objects.filter(id=card.id, title="Card").exists() diff --git a/backend/tasks/tests/test_boards.py b/backend/tasks/tests/test_boards.py index 7e4eb84c..2748b4db 100644 --- a/backend/tasks/tests/test_boards.py +++ b/backend/tasks/tests/test_boards.py @@ -464,7 +464,7 @@ class TestBoardTasks: def test_update_board_task_non_member_forbidden( self, user_client, admin_profile, admin_user, org_a ): - """Non-member should get 403 when updating a board task.""" + """Non-member should get 404, as GET answers them, when updating a board task.""" board = Board.objects.create( name="Private Task Board", owner=admin_profile, @@ -485,7 +485,7 @@ class TestBoardTasks: {"title": "Hacked Card"}, format="json", ) - assert response.status_code == 403 + assert response.status_code == 404 def test_get_tasks_non_member_forbidden( self, user_client, admin_profile, admin_user, org_a @@ -505,7 +505,7 @@ class TestBoardTasks: def test_create_task_non_member_forbidden( self, user_client, admin_profile, admin_user, org_a ): - """Non-member should get 403 when creating a task in a column.""" + """Non-member should get 404, as GET answers them, when creating a task in a column.""" board = Board.objects.create( name="No Access Task Board", owner=admin_profile, @@ -519,7 +519,7 @@ class TestBoardTasks: {"title": "Blocked Card", "priority": "low"}, format="json", ) - assert response.status_code == 403 + assert response.status_code == 404 def test_update_board_task_with_assigned_to( self, admin_client, admin_profile, admin_user, org_a diff --git a/backend/tasks/tests/test_kanban_move_model_errors.py b/backend/tasks/tests/test_kanban_move_model_errors.py new file mode 100644 index 00000000..3e452652 --- /dev/null +++ b/backend/tasks/tests/test_kanban_move_model_errors.py @@ -0,0 +1,70 @@ +"""A task move the model refuses answers 400, as PATCH does, not 500. + +``Task.save()`` calls ``full_clean()``. The PATCH and PUT paths catch the +Django ``ValidationError`` that raises and answer 400 (`_model_errors`); the +board move did not, so any move the model refused was a 500. It is reachable +through configuration alone: ``TaskStage.maps_to_status`` is free text, so a +stage mapped to a status ``Task`` does not have made every move into it crash. +Task moves have no completion side effect to mirror: ``Task`` has no +``completed_at`` and no save signal, so status is the whole of it. +""" + +import pytest + +from tasks.models import Task, TaskPipeline, TaskStage + + +@pytest.mark.django_db +def test_move_into_a_stage_the_model_refuses_is_a_400( + admin_client, admin_profile, admin_user, org_a +): + pipeline = TaskPipeline.objects.create( + name="Delivery", org=org_a, created_by=admin_user + ) + stage = TaskStage.objects.create( + pipeline=pipeline, + name="Shipped", + order=1, + maps_to_status="Shipped", + org=org_a, + created_by=admin_user, + ) + task = Task.objects.create( + title="Pack the box", + status="New", + priority="Low", + org=org_a, + created_by=admin_user, + ) + + response = admin_client.patch( + f"/api/tasks/{task.id}/move/", {"stage_id": str(stage.id)}, format="json" + ) + + assert response.status_code == 400, response.content + body = response.json() + assert body["error"] is True + assert "status" in body["errors"] + task.refresh_from_db() + assert (task.status, task.stage_id) == ("New", None) + + +@pytest.mark.django_db +def test_move_to_completed_still_goes_through( + admin_client, admin_profile, admin_user, org_a +): + task = Task.objects.create( + title="Pack the box", + status="New", + priority="Low", + org=org_a, + created_by=admin_user, + ) + + response = admin_client.patch( + f"/api/tasks/{task.id}/move/", {"status": "Completed"}, format="json" + ) + + assert response.status_code == 200, response.content + task.refresh_from_db() + assert task.status == "Completed" diff --git a/backend/tasks/tests/test_kanban_move_write_rule.py b/backend/tasks/tests/test_kanban_move_write_rule.py new file mode 100644 index 00000000..8c2722ef --- /dev/null +++ b/backend/tasks/tests/test_kanban_move_write_rule.py @@ -0,0 +1,117 @@ +"""The board move takes the task's access rule, exactly (D42). + +`TaskMoveView.patch` rewrites a task's status, stage and kanban order. It used +to gate on ``is_org_admin(...) or request.user.is_superuser`` plus creator or +assignee, while `tasks.access` gives a Django superuser nothing and +`TaskDetailView.patch` refuses them. So a superuser who is a plain member could +move, through the board, any task they could not edit through the detail +endpoint. These tests pin the move to `assert_task_access`. +""" + +import pytest + +from conftest import rls_org +from tasks.models import Task + + +def _move(client, task, status="In Progress"): + return client.patch( + f"/api/tasks/{task.id}/move/", {"status": status}, format="json" + ) + + +@pytest.fixture +def superuser(regular_user): + """The `user_client` caller, still a plain USER profile, made superuser.""" + regular_user.is_superuser = True + regular_user.save(update_fields=["is_superuser"]) + return regular_user + + +def _task(org, created_by, title="A task"): + return Task.objects.create( + title=title, status="New", priority="Low", org=org, created_by=created_by + ) + + +@pytest.fixture +def others_task(admin_user, org_a): + """A task the `user_client` caller neither created nor was handed.""" + return _task(org_a, admin_user, "Someone else's task") + + +def _assert_moved(task): + task.refresh_from_db() + assert task.status == "In Progress" + + +def _assert_unchanged(task): + task.refresh_from_db() + assert task.status == "New" + + +class TestSuperuserPlainMember: + def test_refused_on_a_task_they_cannot_edit( + self, superuser, user_client, others_task + ): + assert ( + user_client.patch( + f"/api/tasks/{others_task.id}/", + {"status": "In Progress"}, + format="json", + ).status_code + == 403 + ) + response = _move(user_client, others_task) + assert response.status_code == 403, response.content + _assert_unchanged(others_task) + + def test_allowed_on_a_task_they_created( + self, superuser, user_client, regular_user, org_a + ): + task = _task(org_a, regular_user, "Mine") + response = _move(user_client, task) + assert response.status_code == 200, response.content + _assert_moved(task) + + def test_allowed_on_a_task_they_are_assigned( + self, superuser, user_client, user_profile, others_task + ): + others_task.assigned_to.add(user_profile) + response = _move(user_client, others_task) + assert response.status_code == 200, response.content + _assert_moved(others_task) + + +class TestOrdinaryRoles: + def test_admin_may_move_any_task(self, admin_client, regular_user, org_a): + task = _task(org_a, regular_user, "A member's task") + response = _move(admin_client, task) + assert response.status_code == 200, response.content + _assert_moved(task) + + def test_member_with_access_may_move(self, user_client, user_profile, others_task): + others_task.assigned_to.add(user_profile) + response = _move(user_client, others_task) + assert response.status_code == 200, response.content + _assert_moved(others_task) + + def test_member_without_access_is_refused(self, user_client, others_task): + response = _move(user_client, others_task) + assert response.status_code == 403, response.content + _assert_unchanged(others_task) + + +@pytest.mark.parametrize("caller", ["member", "admin", "superuser"]) +def test_another_orgs_task_is_404_for_everyone( + caller, request, user_client, admin_client, user_b, org_b +): + if caller == "superuser": + request.getfixturevalue("superuser") + client = admin_client if caller == "admin" else user_client + with rls_org(org_b): + foreign = _task(org_b, user_b, "Foreign") + response = _move(client, foreign) + assert response.status_code == 404, response.content + with rls_org(org_b): + _assert_unchanged(foreign) diff --git a/backend/tasks/tests/test_pipeline_counts_visibility.py b/backend/tasks/tests/test_pipeline_counts_visibility.py new file mode 100644 index 00000000..bb422c72 --- /dev/null +++ b/backend/tasks/tests/test_pipeline_counts_visibility.py @@ -0,0 +1,118 @@ +"""``task_count`` on a task pipeline counts only tasks the caller can open (D47). + +It used to count every task in the org, so a member whose board showed one +card read ``task_count: 2``, a number that included a task they cannot open. +It follows ``tasks.access.visible_tasks_qs`` now, the rule the board and the +list use, in all four places it appears: the pipeline list, the pipeline +detail, each of its nested stages, and the pipeline block on the board. The +same fix D9 made for lead pipelines. +""" + +import pytest +from django.db import connection +from django.test.utils import CaptureQueriesContext + +from conftest import rls_org +from tasks.models import Task, TaskPipeline, TaskStage + + +def _pipeline(org, user, name="Delivery"): + pipeline = TaskPipeline.objects.create(name=name, org=org, created_by=user) + first = TaskStage.objects.create( + pipeline=pipeline, name="To do", order=1, org=org, created_by=user + ) + second = TaskStage.objects.create( + pipeline=pipeline, name="Doing", order=2, org=org, created_by=user + ) + return pipeline, first, second + + +def _task(org, user, title, stage): + return Task.objects.create( + title=title, + status="New", + priority="Low", + org=org, + created_by=user, + stage=stage, + ) + + +@pytest.fixture +def seeded(admin_user, admin_profile, user_profile, org_a, org_b, user_b): + pipeline, first, _second = _pipeline(org_a, admin_user) + _task(org_a, admin_user, "Hidden from the member", first) + mine = _task(org_a, admin_user, "Handed to the member", first) + # Two assignees: a join on the M2M would count this task twice. + mine.assigned_to.add(user_profile, admin_profile) + # Another org's task, in another org's pipeline, never counted here. + with rls_org(org_b): + _b_pipeline, b_stage, _ = _pipeline(org_b, user_b, "Org B") + _task(org_b, user_b, "Org B task", b_stage) + return pipeline + + +def _counts(client, pipeline): + """task_count from the list, the detail, its first stage and the board.""" + (row,) = client.get("/api/tasks/pipelines/").json()["pipelines"] + assert row["id"] == str(pipeline.id) + detail = client.get(f"/api/tasks/pipelines/{pipeline.id}/").json() + board = client.get("/api/tasks/kanban/", {"pipeline_id": str(pipeline.id)}).json() + assert [s["task_count"] for s in detail["stages"][1:]] == [0] + return ( + row["task_count"], + detail["task_count"], + detail["stages"][0]["task_count"], + board["pipeline"]["task_count"], + ) + + +@pytest.mark.django_db +class TestTaskPipelineCountsFollowVisibility: + def test_member_counts_only_visible_tasks(self, user_client, seeded): + assert _counts(user_client, seeded) == (1, 1, 1, 1) + + def test_admin_counts_every_task_in_the_org(self, admin_client, seeded): + assert _counts(admin_client, seeded) == (2, 2, 2, 2) + board = admin_client.get( + "/api/tasks/kanban/", {"pipeline_id": str(seeded.id)} + ).json() + assert board["columns"][0]["task_count"] == 2 + assert board["pipeline"]["stage_count"] == 2 + + def test_member_board_column_and_pipeline_agree(self, user_client, seeded): + board = user_client.get( + "/api/tasks/kanban/", {"pipeline_id": str(seeded.id)} + ).json() + assert board["columns"][0]["task_count"] == 1 + assert len(board["columns"][0]["tasks"]) == 1 + assert board["pipeline"]["task_count"] == 1 + + def test_write_responses_carry_task_count(self, admin_client, admin_user, org_a): + created = admin_client.post( + "/api/tasks/pipelines/", {"name": "Fresh"}, format="json" + ).json() + assert created["task_count"] == 0 + assert {s["task_count"] for s in created["stages"]} == {0} + + _pipeline_obj, first, _second = _pipeline(org_a, admin_user, "Other") + _task(org_a, admin_user, "Staged", first) + updated = admin_client.put( + f"/api/tasks/stages/{first.id}/", {"name": "Renamed"}, format="json" + ).json() + assert updated["task_count"] == 1 + + def test_pipeline_list_query_count_does_not_grow_per_pipeline( + self, admin_client, admin_user, org_a, seeded + ): + with CaptureQueriesContext(connection) as one: + admin_client.get("/api/tasks/pipelines/") + + for name in ("Second", "Third"): + _p, stage, _s = _pipeline(org_a, admin_user, name) + _task(org_a, admin_user, f"{name} task", stage) + with CaptureQueriesContext(connection) as three: + response = admin_client.get("/api/tasks/pipelines/") + + assert len(response.json()["pipelines"]) == 3 + assert len(three.captured_queries) == len(one.captured_queries) diff --git a/backend/tasks/tests/test_task_stage_maps_to_status.py b/backend/tasks/tests/test_task_stage_maps_to_status.py new file mode 100644 index 00000000..0b7f9c3f --- /dev/null +++ b/backend/tasks/tests/test_task_stage_maps_to_status.py @@ -0,0 +1,61 @@ +"""`TaskStage.maps_to_status` must be a task status or empty. + +The column is free text. A stage mapped to anything else is one no task can +move into, because `Task.save()` runs `full_clean()` and refuses the status +the move would set. The serializer now refuses it at the door with a 400. +""" + +import pytest + +from tasks.models import TaskPipeline, TaskStage + + +@pytest.fixture +def pipeline(admin_user, org_a): + return TaskPipeline.objects.create( + name="Stage status pipeline", org=org_a, created_by=admin_user + ) + + +def _create(client, pipeline, **extra): + return client.post( + f"/api/tasks/pipelines/{pipeline.id}/stages/", + {"name": "Stage", "order": 1, "stage_type": "open", **extra}, + format="json", + ) + + +@pytest.mark.django_db +class TestMapsToStatus: + def test_unknown_status_is_400(self, admin_client, pipeline): + resp = _create(admin_client, pipeline, maps_to_status="Shipped") + assert resp.status_code == 400, resp.content + body = resp.json() + assert body["error"] is True + assert "maps_to_status" in body["errors"] + assert not TaskStage.objects.filter(pipeline=pipeline).exists() + + def test_real_status_is_201(self, admin_client, pipeline): + resp = _create(admin_client, pipeline, maps_to_status="In Progress") + assert resp.status_code == 201, resp.content + assert resp.json()["maps_to_status"] == "In Progress" + + @pytest.mark.parametrize("value", ["", None]) + def test_empty_is_allowed(self, admin_client, pipeline, value): + resp = _create(admin_client, pipeline, maps_to_status=value) + assert resp.status_code == 201, resp.content + + def test_update_to_unknown_status_is_400( + self, admin_client, admin_user, org_a, pipeline + ): + stage = TaskStage.objects.create( + pipeline=pipeline, name="Existing", order=1, org=org_a + ) + resp = admin_client.put( + f"/api/tasks/stages/{stage.id}/", + {"maps_to_status": "completed"}, + format="json", + ) + assert resp.status_code == 400, resp.content + stage.refresh_from_db() + assert stage.maps_to_status is None diff --git a/backend/tasks/views/board_views.py b/backend/tasks/views/board_views.py index 6d158fea..95254599 100644 --- a/backend/tasks/views/board_views.py +++ b/backend/tasks/views/board_views.py @@ -66,6 +66,23 @@ def _pop_assignee_ids(data): return payload_id_list(value, "assigned_to_ids") +def _readable_boards(profile): + """Boards ``profile`` may open: its owner or a member, with no org-admin + exception, inside their org. + + Every board, column and card lookup goes through this queryset, so an id + the caller may not open raises the same 404 as an id that does not exist. + Checking access after an org-wide lookup gave the two different answers: + first a 403 on the write verbs, then a 404 with a different body, and + either one told anyone in the org which board and card ids exist. + """ + return ( + Board.objects.filter(org=profile.org) + .filter(Q(owner=profile) | Q(members=profile)) + .distinct() + ) + + def _set_card_assignees(card, org, ids): """Replace the card's assignees with the profiles in ``ids`` who can open its board, and email the ones this write added. @@ -117,16 +134,12 @@ class BoardListCreateView(APIView, LimitOffsetPagination): ) def get(self, request): """List all boards for the user's organization""" - org = request.profile.org user_profile = request.profile # Get boards where user is owner or member queryset = ( - Board.objects.filter( - Q(org=org) & (Q(owner=user_profile) | Q(members=user_profile)) - ) + _readable_boards(user_profile) .prefetch_related("memberships") - .distinct() .order_by("-created_at") ) @@ -204,16 +217,9 @@ class BoardDetailView(APIView): permission_classes = (IsAuthenticated, HasOrgContext) - def get_object(self, pk, org, user_profile): - """Get board if user has access""" - board = get_object_or_404(Board, pk=pk, org=org) - # Check if user is owner or member - if ( - board.owner != user_profile - and not board.members.filter(id=user_profile.id).exists() - ): - return None - return board + def get_object(self, pk, user_profile): + """The board, or 404 when it is missing or the caller may not open it.""" + return get_object_or_404(_readable_boards(user_profile), pk=pk) @extend_schema( tags=["Boards"], @@ -226,12 +232,7 @@ class BoardDetailView(APIView): ) def get(self, request, pk): """Get board details with columns and tasks""" - board = self.get_object(pk, request.profile.org, request.profile) - if not board: - return Response( - {"error": "Board not found or access denied"}, - status=status.HTTP_404_NOT_FOUND, - ) + board = self.get_object(pk, request.profile) serializer = BoardSerializer(board) return Response(serializer.data) @@ -247,12 +248,7 @@ class BoardDetailView(APIView): ) def put(self, request, pk): """Update board""" - board = self.get_object(pk, request.profile.org, request.profile) - if not board: - return Response( - {"error": "Board not found or access denied"}, - status=status.HTTP_404_NOT_FOUND, - ) + board = self.get_object(pk, request.profile) # Only owner or admin can update board membership = BoardMember.objects.filter( @@ -286,12 +282,7 @@ class BoardDetailView(APIView): ) def patch(self, request, pk): """Handle partial updates to a board.""" - board = self.get_object(pk, request.profile.org, request.profile) - if not board: - return Response( - {"error": "Board not found or access denied"}, - status=status.HTTP_404_NOT_FOUND, - ) + board = self.get_object(pk, request.profile) # Only owner or admin can update board membership = BoardMember.objects.filter( @@ -323,12 +314,7 @@ class BoardDetailView(APIView): ) def delete(self, request, pk): """Delete board (owner only)""" - board = self.get_object(pk, request.profile.org, request.profile) - if not board: - return Response( - {"error": "Board not found or access denied"}, - status=status.HTTP_404_NOT_FOUND, - ) + board = self.get_object(pk, request.profile) # Only owner can delete if board.owner != request.profile: @@ -357,18 +343,7 @@ class BoardColumnListCreateView(APIView): ) def get(self, request, board_pk): """List all columns for a board""" - org = request.profile.org - board = get_object_or_404(Board, pk=board_pk, org=org) - - # Check access - if ( - board.owner != request.profile - and not board.members.filter(id=request.profile.id).exists() - ): - return Response( - {"error": "Board not found or access denied"}, - status=status.HTTP_404_NOT_FOUND, - ) + board = get_object_or_404(_readable_boards(request.profile), pk=board_pk) # Prefetch the nested cards with their account + assignees so rendering a # full board (this is the endpoint the kanban reads) stays a handful of @@ -397,7 +372,7 @@ class BoardColumnListCreateView(APIView): def post(self, request, board_pk): """Create a new column""" org = request.profile.org - board = get_object_or_404(Board, pk=board_pk, org=org) + board = get_object_or_404(_readable_boards(request.profile), pk=board_pk) # Check permission membership = BoardMember.objects.filter( @@ -454,18 +429,9 @@ class BoardTaskListCreateView(APIView): ) def get(self, request, column_pk): """List all tasks for a column""" - org = request.profile.org - column = get_object_or_404(BoardColumn, pk=column_pk, board__org=org) - - # Check access - board = column.board - if ( - board.owner != request.profile - and not board.members.filter(id=request.profile.id).exists() - ): - return Response( - {"error": "Access denied"}, status=status.HTTP_404_NOT_FOUND - ) + column = get_object_or_404( + BoardColumn, pk=column_pk, board__in=_readable_boards(request.profile) + ) tasks = column.tasks.all() serializer = BoardTaskSerializer(tasks, many=True) @@ -484,17 +450,9 @@ class BoardTaskListCreateView(APIView): def post(self, request, column_pk): """Create a new task""" org = request.profile.org - column = get_object_or_404(BoardColumn, pk=column_pk, board__org=org) - - # Check access - board = column.board - membership = BoardMember.objects.filter( - board=board, profile=request.profile - ).first() - if not membership: - return Response( - {"error": "Access denied"}, status=status.HTTP_403_FORBIDDEN - ) + column = get_object_or_404( + BoardColumn, pk=column_pk, board__in=_readable_boards(request.profile) + ) data = request.data.copy() try: @@ -536,17 +494,10 @@ class BoardTaskDetailView(APIView): def put(self, request, pk): """Update task (including moving to a different column).""" org = request.profile.org - task = get_object_or_404(BoardTask, pk=pk, column__board__org=org) - - # Check access + task = get_object_or_404( + BoardTask, pk=pk, column__board__in=_readable_boards(request.profile) + ) board = task.column.board - membership = BoardMember.objects.filter( - board=board, profile=request.profile - ).first() - if not membership: - return Response( - {"error": "Access denied"}, status=status.HTTP_403_FORBIDDEN - ) data = request.data.copy() try: @@ -619,18 +570,9 @@ class BoardTaskDetailView(APIView): ) def delete(self, request, pk): """Delete task""" - org = request.profile.org - task = get_object_or_404(BoardTask, pk=pk, column__board__org=org) - - # Check permission - board = task.column.board - membership = BoardMember.objects.filter( - board=board, profile=request.profile - ).first() - if not membership or membership.role not in ["owner", "admin", "member"]: - return Response( - {"error": "Permission denied"}, status=status.HTTP_403_FORBIDDEN - ) + task = get_object_or_404( + BoardTask, pk=pk, column__board__in=_readable_boards(request.profile) + ) task.delete() return Response(status=status.HTTP_204_NO_CONTENT) diff --git a/backend/tasks/views/kanban_views.py b/backend/tasks/views/kanban_views.py index 66122ebc..7cc82bb0 100644 --- a/backend/tasks/views/kanban_views.py +++ b/backend/tasks/views/kanban_views.py @@ -3,6 +3,7 @@ Kanban views for task management. Supports both status-based (default) and custom pipeline-based kanban boards. """ +from django.core.exceptions import ValidationError as DjangoValidationError from django.db import transaction from django.db.models import Q from django.shortcuts import get_object_or_404 @@ -15,6 +16,7 @@ from rest_framework.views import APIView from common.kanban import place_in_column from common.permissions import HasOrgContext, is_org_admin from common.validators import date_param, uuid_param +from tasks.access import assert_task_access, visible_tasks_qs from tasks.models import Task, TaskPipeline, TaskStage from tasks.serializer import ( TaskKanbanCardSerializer, @@ -23,6 +25,7 @@ from tasks.serializer import ( TaskPipelineSerializer, TaskStageSerializer, ) +from tasks.views.task_views import _model_errors class TaskKanbanView(APIView): @@ -117,18 +120,19 @@ class TaskKanbanView(APIView): .prefetch_related("assigned_to", "tags") ) - # Apply permission filtering - if not is_org_admin(request.profile) and not request.user.is_superuser: - queryset = queryset.filter( - Q(assigned_to=request.profile) | Q(created_by=request.profile.user) - ) + # The task read rule itself, as the list and the detail view apply it: + # no superuser clause, because the detail view has none. By id, so a + # card with several assignees appears once. + queryset = queryset.filter( + pk__in=visible_tasks_qs(request.profile).values("pk") + ) # Apply search/filters queryset = self._apply_filters(queryset, request.query_params) if pipeline_id: # Pipeline-based kanban - return self._get_pipeline_kanban(queryset, pipeline_id, org) + return self._get_pipeline_kanban(queryset, pipeline_id, request) # Status-based kanban return self._get_status_kanban(queryset) @@ -202,10 +206,15 @@ class TaskKanbanView(APIView): } ) - def _get_pipeline_kanban(self, queryset, pipeline_id, org): + def _get_pipeline_kanban(self, queryset, pipeline_id, request): """Build kanban data using TaskPipeline stages as columns.""" pipeline = get_object_or_404( - TaskPipeline, pk=pipeline_id, org=org, is_active=True + TaskPipelineListSerializer.with_counts( + TaskPipeline.objects.all(), request.profile + ), + pk=pipeline_id, + org=request.profile.org, + is_active=True, ) # Filter tasks to this pipeline @@ -258,15 +267,11 @@ class TaskMoveView(APIView): # edit committing between this read and that save would be lost. task = get_object_or_404(Task.objects.select_for_update(), pk=pk, org=org) - # Permission check - if not is_org_admin(request.profile) and not request.user.is_superuser: - if not ( - request.profile.user == task.created_by - or request.profile in task.assigned_to.all() - ): - return Response( - {"error": "Permission denied"}, status=status.HTTP_403_FORBIDDEN - ) + # A move rewrites the task's status, stage and order, so it takes the + # task's own access rule and nothing wider. This used to add a Django + # superuser clause, which let a superuser who is a plain member of the + # org move any task here that `TaskDetailView.patch` refuses them. + assert_task_access(request.profile, task) serializer = TaskMoveSerializer(data=request.data) if not serializer.is_valid(): @@ -316,7 +321,16 @@ class TaskMoveView(APIView): exclude_pk=task.pk, ) - task.save() + # `Task.save()` runs `full_clean()`, which refuses, for one, a stage + # whose free-text `maps_to_status` is not a task status. PATCH answers + # that refusal 400 with the model's own errors; so does the move. + try: + task.save() + except DjangoValidationError as exc: + return Response( + {"error": True, "errors": _model_errors(exc)}, + status=status.HTTP_400_BAD_REQUEST, + ) return Response( { @@ -351,7 +365,9 @@ class TaskPipelineListCreateView(APIView): def get(self, request): """List all pipelines for the organization.""" org = request.profile.org - pipelines = TaskPipeline.objects.filter(org=org, is_active=True) + pipelines = TaskPipelineListSerializer.with_counts( + TaskPipeline.objects.filter(org=org, is_active=True), request.profile + ) serializer = TaskPipelineListSerializer(pipelines, many=True) return Response({"pipelines": serializer.data}) @@ -416,7 +432,8 @@ class TaskPipelineListCreateView(APIView): TaskStage.objects.create(pipeline=pipeline, org=org, **stage_data) return Response( - TaskPipelineSerializer(pipeline).data, status=status.HTTP_201_CREATED + TaskPipelineSerializer(pipeline, context={"request": request}).data, + status=status.HTTP_201_CREATED, ) @@ -432,7 +449,9 @@ class TaskPipelineDetailView(APIView): def get(self, request, pk): """Get pipeline details with all stages.""" pipeline = self.get_object(pk, request.profile.org) - return Response(TaskPipelineSerializer(pipeline).data) + return Response( + TaskPipelineSerializer(pipeline, context={"request": request}).data + ) @extend_schema( tags=["Task Pipelines"], @@ -456,7 +475,9 @@ class TaskPipelineDetailView(APIView): ) pipeline = serializer.save(updated_by=request.user) - return Response(TaskPipelineSerializer(pipeline).data) + return Response( + TaskPipelineSerializer(pipeline, context={"request": request}).data + ) @extend_schema(tags=["Task Pipelines"], responses={204: None}) def delete(self, request, pk): @@ -511,7 +532,10 @@ class TaskStageCreateView(APIView): ) stage = serializer.save(pipeline=pipeline, org=org, created_by=request.user) - return Response(TaskStageSerializer(stage).data, status=status.HTTP_201_CREATED) + return Response( + TaskStageSerializer(stage, context={"request": request}).data, + status=status.HTTP_201_CREATED, + ) class TaskStageDetailView(APIView): @@ -541,7 +565,7 @@ class TaskStageDetailView(APIView): ) stage = serializer.save(updated_by=request.user) - return Response(TaskStageSerializer(stage).data) + return Response(TaskStageSerializer(stage, context={"request": request}).data) @extend_schema(tags=["Task Stages"], responses={204: None}) def delete(self, request, pk): diff --git a/backend/tasks/views/task_views.py b/backend/tasks/views/task_views.py index d79bd9ab..77cb0e78 100644 --- a/backend/tasks/views/task_views.py +++ b/backend/tasks/views/task_views.py @@ -13,7 +13,7 @@ from rest_framework.permissions import IsAuthenticated from rest_framework.response import Response from rest_framework.views import APIView -from accounts.models import Account +from accounts.access import visible_accounts_qs from accounts.serializer import AccountPickerSerializer from common.custom_fields import validate_payload as validate_custom_fields_payload from common.models import ( @@ -40,8 +40,7 @@ from common.validators import ( uuid_list_param, uuid_param, ) -from contacts.access import replace_visible_contacts -from contacts.models import Contact +from contacts.access import replace_visible_contacts, visible_contacts_qs from contacts.serializer import ContactPickerSerializer from tasks import swagger_params from tasks.access import ( @@ -225,19 +224,15 @@ class TaskListView(APIView, LimitOffsetPagination): context["users"] = list(profiles.values("id", "user__email")) # The catalogues are for the parent picker and grow with the org, not # with the page, `?slim=true` omits them for callers that only want - # the list. A non-admin is offered only the accounts and contacts they - # created or are assigned to, as `/api/cases/` and - # `/api/opportunities/` already did: this endpoint used to hand every - # member the whole org's accounts and contacts in full. + # the list. Each offers exactly the records its own detail view would + # open for this caller, by calling that module's read rule: accounts + # to admins, superusers, the creator and assignees; contacts to the + # same, plus anyone assigned to one of the contact's accounts. This + # endpoint used to hand every member the whole org's accounts and + # contacts in full. if params.get("slim") != "true": - accounts = Account.objects.filter(org=self.request.profile.org) - contacts = Contact.objects.filter(org=self.request.profile.org) - if not is_org_admin(self.request.profile): - member_scope = Q(created_by=self.request.profile.user) | Q( - assigned_to=self.request.profile - ) - accounts = accounts.filter(member_scope).distinct() - contacts = contacts.filter(member_scope).distinct() + accounts = visible_accounts_qs(self.request.profile, self.request.user) + contacts = visible_contacts_qs(self.request.profile) context["accounts_list"] = AccountPickerSerializer(accounts, many=True).data context["contacts_list"] = ContactPickerSerializer(contacts, many=True).data return context diff --git a/frontend/src/lib/server/v2/contacts.js b/frontend/src/lib/server/v2/contacts.js index be96ea80..0b142808 100644 --- a/frontend/src/lib/server/v2/contacts.js +++ b/frontend/src/lib/server/v2/contacts.js @@ -386,9 +386,24 @@ export async function getContactForEdit({ cookies }, id) { const raw = response.contact_obj; const contact = toRow(raw); + /* + * The linked account is always an option, even when the picker does not + * carry it: an account the caller cannot open (the list holds only the ones + * they can), a closed one, or one past the 200 cap. A select whose value + * matches no option shows nothing and submits nothing or its first entry, so + * a save would unlink it. The server keeps an unchanged link whoever saves, + * and refuses a newly chosen account the caller cannot open. + */ + const stored = raw.account_detail; + const accounts = + stored && !choices.accounts.some((/** @type {any} */ row) => row.id === stored.id) + ? [{ id: stored.id, name: stored.name }, ...choices.accounts] + : choices.accounts; + return { contact, ...choices, + accounts, form: { first_name: contact.first_name, last_name: contact.last_name, @@ -477,7 +492,7 @@ export async function updateContact({ cookies }, id, values) { * * No `org` and no `created_by` in the body: `ContactsListView.post` sets both * from `request.profile`, and `CreateContactSerializer` lists neither as a - * field. `account` is checked against the caller's org before it is accepted. + * field. `account` must be one the caller can open, or it is refused. * * The response now carries the new `id`, which it did not before this change. * Without it a client cannot open what it just made. diff --git a/frontend/src/lib/server/v2/contacts.test.js b/frontend/src/lib/server/v2/contacts.test.js index 7e52b857..86dbd8c0 100644 --- a/frontend/src/lib/server/v2/contacts.test.js +++ b/frontend/src/lib/server/v2/contacts.test.js @@ -87,3 +87,58 @@ describe('getContactForEdit deal count', () => { expect(server.deal_count).toBe(10); }); }); + +describe('getContactForEdit account picker', () => { + beforeEach(() => { + apiRequest.mockReset(); + }); + + /** @param {any} contactObj */ + function respond(contactObj) { + apiRequest.mockImplementation(async (/** @type {string} */ url) => { + if (url.startsWith('/contacts/c1/')) + return { ...detail({ count: 0, by_currency: [] }), contact_obj: contactObj }; + if (url.startsWith('/accounts/')) + return { + active_accounts: { + open_accounts: [{ id: 'a-visible', name: 'Visible Co' }], + open_accounts_count: 1 + } + }; + return {}; + }); + } + + const base = { id: 'c1', first_name: 'Ada', last_name: 'Lovelace', assigned_to: [], teams: [] }; + + it('offers a linked account the picker list does not carry, so a save keeps it', async () => { + respond({ + ...base, + account: 'a-hidden', + account_detail: { id: 'a-hidden', name: 'Hidden Holdings' } + }); + const { accounts, form } = await getContactForEdit(event, 'c1'); + expect(form.account).toBe('a-hidden'); + expect(accounts).toEqual([ + { id: 'a-hidden', name: 'Hidden Holdings' }, + { id: 'a-visible', name: 'Visible Co' } + ]); + }); + + it('does not repeat a linked account the list already carries', async () => { + respond({ + ...base, + account: 'a-visible', + account_detail: { id: 'a-visible', name: 'Visible Co' } + }); + const { accounts } = await getContactForEdit(event, 'c1'); + expect(accounts).toEqual([{ id: 'a-visible', name: 'Visible Co' }]); + }); + + it('offers only the list when the contact has no account', async () => { + respond({ ...base, account: null, account_detail: null }); + const { accounts, form } = await getContactForEdit(event, 'c1'); + expect(form.account).toBe(''); + expect(accounts).toEqual([{ id: 'a-visible', name: 'Visible Co' }]); + }); +}); diff --git a/frontend/src/lib/server/v2/tickets-parent.test.js b/frontend/src/lib/server/v2/tickets-parent.test.js new file mode 100644 index 00000000..d3a01d83 --- /dev/null +++ b/frontend/src/lib/server/v2/tickets-parent.test.js @@ -0,0 +1,60 @@ +/** + * The parent a ticket sits under, as the list and the detail load read it. + * + * `parent_summary` names the parent only when the viewer may open it. A hidden + * parent arrives as `{ id, name: null, status: null, restricted: true }` (D51, + * the same redaction `/tree/` applies to a hidden node), and must read as the + * same phrase the tree uses rather than as an empty or null name. + */ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { RESTRICTED_TICKET_NAME } from '$lib/v2/enums.js'; + +const apiRequest = vi.fn(); +vi.mock('$lib/api-helpers.js', () => ({ apiRequest: (...a) => apiRequest(...a) })); + +const { getTicket, listTickets } = await import('./tickets.js'); + +const event = /** @type {any} */ ({ cookies: { get: () => 'token' } }); + +const HIDDEN = { id: 'p-hidden', name: null, status: null, restricted: true }; +const READABLE = { id: 'p-open', name: 'Outage umbrella', status: 'Pending', restricted: false }; + +/** @param {any} parent_summary */ +function row(id, parent_summary) { + return { id, name: `Ticket ${id}`, status: 'New', priority: 'Low', parent_summary }; +} + +describe('a ticket parent', () => { + beforeEach(() => { + apiRequest.mockReset(); + }); + + it('maps each list row on its own parent', async () => { + apiRequest.mockResolvedValue({ + cases: [row('a', HIDDEN), row('b', READABLE), row('c', null)] + }); + const { results } = await listTickets(event); + + expect(results.map((t) => t.parent)).toEqual([ + { id: 'p-hidden', name: RESTRICTED_TICKET_NAME, status: null, restricted: true }, + { id: 'p-open', name: 'Outage umbrella', status: 'Pending', restricted: false }, + null + ]); + }); + + it('never renders a hidden parent as a blank name on the detail', async () => { + apiRequest.mockResolvedValue({ cases_obj: row('a', HIDDEN), comment_permission: false }); + const { ticket } = await getTicket(event, 'a'); + + expect(ticket.parent).toEqual({ + id: 'p-hidden', + name: RESTRICTED_TICKET_NAME, + status: null, + restricted: true + }); + }); + + it('is the phrase the tree uses for a hidden node', () => { + expect(RESTRICTED_TICKET_NAME).toBe('A ticket you cannot open'); + }); +}); diff --git a/frontend/src/lib/server/v2/tickets.js b/frontend/src/lib/server/v2/tickets.js index ac8a83eb..d8d86121 100644 --- a/frontend/src/lib/server/v2/tickets.js +++ b/frontend/src/lib/server/v2/tickets.js @@ -46,6 +46,7 @@ import { error, redirect } from '@sveltejs/kit'; import { apiRequest } from '$lib/api-helpers.js'; import { attachmentHref } from '$lib/server/v2/files.js'; +import { RESTRICTED_TICKET_NAME } from '$lib/v2/enums.js'; /** Statuses where somebody still owes the customer something. Mirrors `cases.views.OPEN_STATUSES`. */ export const OPEN_STATUSES = ['New', 'Assigned', 'Pending']; @@ -64,6 +65,29 @@ function accountLink(account) { return account ? { id: account.id, name: account.name ?? '' } : null; } +/** + * The ticket this one sits under, or null. + * + * A parent the viewer may not open arrives as `{ id, name: null, status: null, + * restricted: true }`. It keeps its id (the viewer may still detach from it, + * since unlinking needs write on the child alone) and reads as + * `RESTRICTED_TICKET_NAME`, never as a link: opening it would only answer 403. + * + * @param {any} parent + */ +function parentLink(parent) { + if (!parent) return null; + if (parent.restricted) { + return { id: parent.id, name: RESTRICTED_TICKET_NAME, status: null, restricted: true }; + } + return { + id: parent.id, + name: parent.name ?? '', + status: parent.status ?? null, + restricted: false + }; +} + /** * The subset of a Django case the v2 pages read. * @@ -110,7 +134,7 @@ function toRow(row) { escalation_count: row.escalation_count ?? 0, paused_at: row.sla_paused_at ?? null, child_count: row.child_count ?? 0, - parent: row.parent_summary ?? null, + parent: parentLink(row.parent_summary), // Everyone's logged time on this ticket, which is not what // `/cases//time-entries/` returns to an agent: that list is narrowed // to their own rows, so a panel adding it up would tell a team of three diff --git a/frontend/src/lib/server/v2/today.js b/frontend/src/lib/server/v2/today.js index 90b35ccb..90c02328 100644 --- a/frontend/src/lib/server/v2/today.js +++ b/frontend/src/lib/server/v2/today.js @@ -5,8 +5,9 @@ * sources into a single prioritised action list: support cases still awaiting a * first reply, invoices past due, deals that have gone quiet (stage-aging), and * tasks overdue or due today. The endpoint is org-scoped from the JWT and, for a - * member, restricted to rows assigned to or created by them, so nothing here - * decides visibility; the API does. + * member, narrowed to rows whose action they can take (a ticket they only watch + * is left out, since they cannot reply to it), so nothing here decides + * visibility; the API does. * * This is read-only: the home page links out to each item's own module, where * the writes already live. There is no write path to wire here. diff --git a/frontend/src/lib/v2/enums.js b/frontend/src/lib/v2/enums.js index 973c8c5a..b9b90dc5 100644 --- a/frontend/src/lib/v2/enums.js +++ b/frontend/src/lib/v2/enums.js @@ -176,6 +176,15 @@ export const CASE_STATUS_TONE = { Duplicate: 'slate' }; +/** + * What a ticket the viewer may not open is called wherever it still appears. + * + * The API keeps such a ticket in place, by id, with `restricted: true` and no + * name: a node in `/tree/`, or a ticket's `parent_summary`. Shared so the tree, + * the close confirm step and the parent link all say the same thing. + */ +export const RESTRICTED_TICKET_NAME = 'A ticket you cannot open'; + export const INVOICE_STATUS_TONE = { Draft: 'slate', Sent: 'ink', diff --git a/frontend/src/routes/(app)/tickets/[id]/+page.server.js b/frontend/src/routes/(app)/tickets/[id]/+page.server.js index bbfdcf6f..4034fbf8 100644 --- a/frontend/src/routes/(app)/tickets/[id]/+page.server.js +++ b/frontend/src/routes/(app)/tickets/[id]/+page.server.js @@ -166,8 +166,10 @@ export const actions = { * PATCHes the case; this posts to `close-with-children/`, which closes the * subtree in one transaction and writes a `PARENT_CLOSED_CASCADE` activity * row on each child. Folding the two together would mean a ticket with no - * children took the heavier path for no reason, and the approval gate on the - * ordinary close lives on the PATCH. + * children took the heavier path for no reason. Both take the same approval + * gate; this one also refuses the whole cascade (nothing closed) when any + * ticket it would take is not the viewer's to close, and the refusal lands + * in `form.error` like any other. * * `cascade` is read from the checkbox, so an unticked box sends `false` and * closes the parent alone. It is never omitted: the API reads the org diff --git a/frontend/src/routes/(app)/tickets/[id]/close.js b/frontend/src/routes/(app)/tickets/[id]/close.js index e1bc88f6..9b9493de 100644 --- a/frontend/src/routes/(app)/tickets/[id]/close.js +++ b/frontend/src/routes/(app)/tickets/[id]/close.js @@ -25,9 +25,21 @@ * closed child is still cascaded. */ +import { RESTRICTED_TICKET_NAME } from '$lib/v2/enums.js'; + /** A ticket closes to any status but this one. */ const CLOSED = 'Closed'; +/** + * What a ticket the viewer may not open is called in the list. + * + * `/tree/` redacts such a node to `{ id, name: null, restricted: true, status, + * is_active }`: kept, so the count and the shape stay true, but nameless. The + * close refuses the whole cascade when one of these would be closed, so the + * list should say plainly that it is there. + */ +export const RESTRICTED_NAME = RESTRICTED_TICKET_NAME; + /** * The node for `id` inside the tree, or null. * @@ -49,13 +61,13 @@ export function findNode(root, id) { * * @param {any} root the tree response's `root` * @param {string} id the ticket being closed - * @returns {Array<{id: string, name: string, status: string}>} + * @returns {Array<{id: string, name: string, status: string, restricted: boolean}>} */ export function openDescendants(root, id) { const focus = findNode(root, id); if (!focus) return []; - /** @type {Array<{id: string, name: string, status: string}>} */ + /** @type {Array<{id: string, name: string, status: string, restricted: boolean}>} */ const out = []; /** @param {any} node */ const walk = (node) => { @@ -63,7 +75,12 @@ export function openDescendants(root, id) { // Open AND active, matching `_open_descendants`. An inactive row is a // merged duplicate and the backend skips it. if (child.status !== CLOSED && child.is_active !== false) { - out.push({ id: child.id, name: child.name, status: child.status }); + out.push({ + id: child.id, + name: child.restricted ? RESTRICTED_NAME : child.name, + status: child.status, + restricted: Boolean(child.restricted) + }); } // Through closed children regardless: an open grandchild under a closed // child still cascades. diff --git a/frontend/src/routes/(app)/tickets/[id]/close.test.js b/frontend/src/routes/(app)/tickets/[id]/close.test.js index 52b04580..b688c0f3 100644 --- a/frontend/src/routes/(app)/tickets/[id]/close.test.js +++ b/frontend/src/routes/(app)/tickets/[id]/close.test.js @@ -5,7 +5,8 @@ import { subtreeTruncated, cascadeSummary, closeResultMessage, - cascadedCount + cascadedCount, + RESTRICTED_NAME } from './close.js'; /** @param {any} over */ @@ -93,6 +94,20 @@ describe('openDescendants', () => { expect(openDescendants(tree, 'p').map((d) => d.id)).toEqual(['real']); }); + it('lists a ticket the viewer cannot open, without a name', () => { + // `/tree/` redacts it to an id, a status and `restricted`. It still + // counts, because the close refuses a cascade that would take it. + const tree = node({ + id: 'p', + children: [ + { id: 'hidden', name: null, restricted: true, status: 'New', is_active: true, children: [] } + ] + }); + expect(openDescendants(tree, 'p')).toEqual([ + { id: 'hidden', name: RESTRICTED_NAME, status: 'New', restricted: true } + ]); + }); + it('is empty for a leaf, and for a ticket not in the tree', () => { expect(openDescendants(TREE, 'kidOpen')).toEqual([]); expect(openDescendants(TREE, 'nope')).toEqual([]); diff --git a/mobile/docs/PARITY.md b/mobile/docs/PARITY.md index 4aa5f347..b7e2eed7 100644 --- a/mobile/docs/PARITY.md +++ b/mobile/docs/PARITY.md @@ -20,6 +20,7 @@ On 2026-09-26 the counts were 90 `+page.svelte` files under `frontend/src/routes | ID | Gap | Missing on | Backend | Opened | Notes | |---|---|---|---|---|---| | B1 | CSV import for contacts, tickets and leads | Mobile | `import/preview/` and `import/commit/` exist | 2026-09-26 | Web mounted `ContactImportDrawer` and `TicketImportDrawer` on 2026-09-26, and `LeadImportDrawer` (`/api/leads/import/`) the same day. Tracked as G1 in `enterprise-crm/docs/gap-analysis/TRACKER.md`. | +| B3 | Parent and child tickets: parent banner, link parent, detach, tree | Web | `/api/cases//tree/`, `link/`, `close-with-children/` and `parent_summary` exist (authz fixed as D50 and D51) | 2026-09-26 | Mobile has all of it in `ticket_detail_screen.dart`. Web has `TicketTreePanel.svelte` and `LinkParentDialog.svelte` exported from the components index but imported nowhere; only the close-with-children cascade (`tickets/[id]/close.js`) is wired. | ## Web-only by design diff --git a/mobile/lib/data/models/ticket.dart b/mobile/lib/data/models/ticket.dart index 032cabe6..1763910d 100644 --- a/mobile/lib/data/models/ticket.dart +++ b/mobile/lib/data/models/ticket.dart @@ -461,22 +461,33 @@ class Ticket { } /// Lightweight ref to a parent ticket, used to render the "linked to" tile. +/// +/// A parent the viewer may not open arrives as `restricted: true` with no name +/// or status, the redaction `/tree/` applies to a hidden node. [name] then +/// reads as [restrictedName] and the screen does not offer to open it. The id +/// stays, since detaching from it needs write on the child alone. class TicketParentSummary { + static const restrictedName = 'A ticket you cannot open'; + final String id; final String name; final String? status; + final bool restricted; const TicketParentSummary({ required this.id, required this.name, this.status, + this.restricted = false, }); factory TicketParentSummary.fromJson(Map json) { + final restricted = json['restricted'] == true; return TicketParentSummary( id: json['id']?.toString() ?? '', - name: json['name']?.toString() ?? '', - status: json['status'] as String?, + name: restricted ? restrictedName : json['name']?.toString() ?? '', + status: restricted ? null : json['status'] as String?, + restricted: restricted, ); } } diff --git a/mobile/lib/providers/tickets_provider.dart b/mobile/lib/providers/tickets_provider.dart index 0a7617a9..de0f1cc1 100644 --- a/mobile/lib/providers/tickets_provider.dart +++ b/mobile/lib/providers/tickets_provider.dart @@ -661,7 +661,14 @@ class TicketDetailResult { } /// Recursive tree node returned by `/api/cases/{id}/tree/`. +/// +/// A ticket the viewer may not open comes back redacted: `restricted: true`, +/// `name: null`, and only its id, status and active flag. It stays in the tree +/// so the shape and the cascade count are true; [name] reads as +/// [restrictedName] for it, and the screen does not offer to open it. class TicketTreeNode { + static const restrictedName = TicketParentSummary.restrictedName; + final String id; final String name; final String? status; @@ -669,6 +676,7 @@ class TicketTreeNode { final bool isProblem; final bool isActive; final bool truncated; + final bool restricted; final List children; final String? focusId; @@ -680,6 +688,7 @@ class TicketTreeNode { this.isProblem = false, this.isActive = true, this.truncated = false, + this.restricted = false, this.children = const [], this.focusId, }); @@ -690,14 +699,16 @@ class TicketTreeNode { Map json, { String? focusId, }) { + final restricted = json['restricted'] == true; return TicketTreeNode( id: json['id']?.toString() ?? '', - name: json['name']?.toString() ?? '', + name: restricted ? restrictedName : json['name']?.toString() ?? '', status: json['status'] as String?, priority: json['priority'] as String?, isProblem: json['is_problem'] as bool? ?? false, isActive: json['is_active'] as bool? ?? true, truncated: json['truncated'] as bool? ?? false, + restricted: restricted, children: ((json['children'] as List?) ?? const []) .whereType>() .map((c) => TicketTreeNode.fromJson(c, focusId: focusId)) diff --git a/mobile/lib/screens/contacts/contact_form_screen.dart b/mobile/lib/screens/contacts/contact_form_screen.dart index 73589180..f5ffaba6 100644 --- a/mobile/lib/screens/contacts/contact_form_screen.dart +++ b/mobile/lib/screens/contacts/contact_form_screen.dart @@ -334,7 +334,24 @@ class _ContactFormScreenState extends ConsumerState { /// choice, because a select whose value matches no option submits its first /// entry, which would attach the contact to whichever company happens to /// sort first. - Widget _accountPicker(List accounts) { + /// + /// The account the contact is already linked to is always an item, even + /// when the lookup does not carry it: one the user cannot open (the lookup + /// holds only those they can), a closed one, or one past the lookup cap. + /// Otherwise the picker reads "No account" while the save keeps the link. + /// The server keeps an unchanged link whoever saves, and refuses a newly + /// chosen account the user cannot open. + Widget _accountPicker(List lookup) { + final storedId = _existing?.accountId; + final accounts = storedId == null || lookup.any((a) => a.id == storedId) + ? lookup + : [ + AccountLookup( + id: storedId, + name: _existing?.accountName ?? 'Current account', + ), + ...lookup, + ]; final ids = accounts.map((a) => a.id).toSet(); final value = ids.contains(_accountId) ? _accountId : null; return Padding( diff --git a/mobile/lib/screens/tickets/ticket_detail_screen.dart b/mobile/lib/screens/tickets/ticket_detail_screen.dart index 9f3da256..bcb35e77 100644 --- a/mobile/lib/screens/tickets/ticket_detail_screen.dart +++ b/mobile/lib/screens/tickets/ticket_detail_screen.dart @@ -1199,11 +1199,16 @@ class _TicketDetailScreenState extends ConsumerState children: [ Icon(icon, size: 20, color: iconColor ?? AppColors.textSecondary), const SizedBox(width: 16), - Text( - label, - style: AppTypography.body.copyWith( - color: labelColor ?? AppColors.textPrimary, - fontWeight: FontWeight.w500, + // Expanded so a long label wraps instead of overflowing: the + // detach row names the parent, and a ticket subject has no limit + // a 390px sheet can hold. + Expanded( + child: Text( + label, + style: AppTypography.body.copyWith( + color: labelColor ?? AppColors.textPrimary, + fontWeight: FontWeight.w500, + ), ), ), ], @@ -1636,7 +1641,10 @@ class _TicketDetailScreenState extends ConsumerState const SizedBox(height: 12), if (c.parentSummary != null) InkWell( - onTap: () => context.push('/tickets/${c.parentSummary!.id}'), + // A parent the viewer cannot open would only answer 403. + onTap: c.parentSummary!.restricted + ? null + : () => context.push('/tickets/${c.parentSummary!.id}'), child: Padding( padding: const EdgeInsets.symmetric(vertical: 6), child: Row( @@ -1661,6 +1669,12 @@ class _TicketDetailScreenState extends ConsumerState c.parentSummary!.name, style: AppTypography.body.copyWith( fontWeight: FontWeight.w500, + fontStyle: c.parentSummary!.restricted + ? FontStyle.italic + : FontStyle.normal, + color: c.parentSummary!.restricted + ? AppColors.textTertiary + : null, ), ), ], @@ -1711,7 +1725,10 @@ class _TicketDetailScreenState extends ConsumerState if (depth > 0) { rows.add( InkWell( - onTap: () => context.push('/tickets/${node.id}'), + // A ticket the viewer cannot open would only answer 403. + onTap: node.restricted + ? null + : () => context.push('/tickets/${node.id}'), child: Padding( padding: EdgeInsets.fromLTRB((depth - 1) * 16.0, 6, 0, 6), child: Row( @@ -1729,8 +1746,13 @@ class _TicketDetailScreenState extends ConsumerState fontWeight: node.isFocused ? FontWeight.w700 : FontWeight.w500, + fontStyle: node.restricted + ? FontStyle.italic + : FontStyle.normal, color: node.isFocused ? AppColors.primary600 + : node.restricted + ? AppColors.textTertiary : AppColors.textPrimary, ), ), @@ -1785,7 +1807,11 @@ class _TicketDetailScreenState extends ConsumerState _actionRow( icon: LucideIcons.unlink, iconColor: AppColors.danger600, - label: 'Detach from "${c.parentSummary!.name}"', + // Unlinking needs write on this ticket only, so a hidden + // parent can still be left; it just is not named. + label: c.parentSummary!.restricted + ? 'Detach from parent ticket' + : 'Detach from "${c.parentSummary!.name}"', labelColor: AppColors.danger600, onTap: () => Navigator.pop(context, 'detach'), ), diff --git a/mobile/test/screens/contacts/contact_form_account_test.dart b/mobile/test/screens/contacts/contact_form_account_test.dart new file mode 100644 index 00000000..be747537 --- /dev/null +++ b/mobile/test/screens/contacts/contact_form_account_test.dart @@ -0,0 +1,131 @@ +import 'package:bottle_crm/core/theme/theme.dart'; +import 'package:bottle_crm/data/models/contact.dart'; +import 'package:bottle_crm/data/models/lookup_models.dart'; +import 'package:bottle_crm/providers/contacts_provider.dart'; +import 'package:bottle_crm/providers/lookup_provider.dart'; +import 'package:bottle_crm/screens/contacts/contact_form_screen.dart'; +import 'package:flutter/material.dart'; +import 'package:flutter_riverpod/flutter_riverpod.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:go_router/go_router.dart'; + +/// The contact's linked account stays on the picker and in the save. +/// +/// The accounts lookup carries only the accounts the user can open, so a +/// contact an admin linked to some other company has an `account` the picker +/// does not list. The picker used to read "No account" for it. The save still +/// sent the id, which the server keeps as an unchanged link (D39), so the +/// screen said one thing and the record another. +void main() { + TestWidgetsFlutterBinding.ensureInitialized(); + + const visible = AccountLookup(id: 'a-visible', name: 'Visible Co'); + + Contact contact({String? accountId, String? accountName}) => + Contact.fromJson({ + 'id': 'c1', + 'first_name': 'Ada', + 'last_name': 'Lovelace', + 'account': accountId, + 'account_detail': accountId == null + ? null + : {'id': accountId, 'name': accountName}, + }); + + Future<_FakeContacts> open(WidgetTester tester, Contact stored) async { + final fake = _FakeContacts(stored); + await tester.pumpWidget( + ProviderScope( + overrides: [ + contactsProvider.overrideWith(() => fake), + accountOptionsProvider.overrideWithValue(const [visible]), + ], + child: MaterialApp.router( + theme: AppTheme.light, + routerConfig: GoRouter( + initialLocation: '/contacts/edit', + routes: [ + GoRoute( + path: '/contacts', + builder: (_, _) => const Text('list'), + routes: [ + GoRoute( + path: 'edit', + builder: (_, _) => const ContactFormScreen(contactId: 'c1'), + ), + ], + ), + ], + ), + ), + ), + ); + await tester.pumpAndSettle(); + return fake; + } + + Future save(WidgetTester tester) async { + final button = find.text('Save changes'); + await tester.ensureVisible(button); + await tester.tap(button); + await tester.pumpAndSettle(); + } + + testWidgets('shows and keeps a linked account the lookup does not carry', ( + tester, + ) async { + final fake = await open( + tester, + contact(accountId: 'a-hidden', accountName: 'Hidden Holdings'), + ); + expect(find.text('Hidden Holdings'), findsOneWidget); + expect(find.text('No account'), findsNothing); + + await save(tester); + expect(fake.updates.single['account'], 'a-hidden'); + }); + + testWidgets('a linked account the lookup carries is listed once', ( + tester, + ) async { + final fake = await open( + tester, + contact(accountId: 'a-visible', accountName: 'Visible Co'), + ); + expect(find.text('Visible Co'), findsOneWidget); + + await save(tester); + expect(fake.updates.single['account'], 'a-visible'); + }); + + testWidgets('no linked account reads as No account and sends none', ( + tester, + ) async { + final fake = await open(tester, contact()); + expect(find.text('No account'), findsOneWidget); + + await save(tester); + expect(fake.updates.single['account'], isNull); + }); +} + +class _FakeContacts extends ContactsNotifier { + _FakeContacts(this.stored); + + final Contact stored; + + /// Every update body, in order. + final List> updates = []; + + @override + Future build() async => const ContactsListData(); + + @override + Future getContact(String id) async => stored; + + @override + Future updateContact(String id, Map payload) async { + updates.add(payload); + return null; + } +} diff --git a/mobile/test/screens/tickets/ticket_parent_restricted_test.dart b/mobile/test/screens/tickets/ticket_parent_restricted_test.dart new file mode 100644 index 00000000..afcc01ed --- /dev/null +++ b/mobile/test/screens/tickets/ticket_parent_restricted_test.dart @@ -0,0 +1,227 @@ +import 'package:bottle_crm/core/theme/theme.dart'; +import 'package:bottle_crm/data/models/ticket.dart'; +import 'package:bottle_crm/providers/lookup_provider.dart'; +import 'package:bottle_crm/providers/tickets_provider.dart'; +import 'package:bottle_crm/screens/tickets/ticket_detail_screen.dart'; +import 'package:bottle_crm/widgets/cards/ticket_card.dart'; +import 'package:flutter/material.dart'; +import 'package:flutter_riverpod/flutter_riverpod.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:go_router/go_router.dart'; +import 'package:lucide_icons_flutter/lucide_icons.dart'; + +/// A ticket whose parent the viewer may not open (D51). +/// +/// `parent_summary` used to carry the parent's name to anyone who could read +/// the child. The API now sends `{id, name: null, status: null, restricted: +/// true}` for a hidden parent, the redaction `/tree/` uses for a hidden node, +/// and the phone must say "A ticket you cannot open", not offer to open it +/// (that would only answer 403), and still let the viewer detach from it, +/// since unlinking needs write on the child alone. +void main() { + TestWidgetsFlutterBinding.ensureInitialized(); + + const hiddenJson = { + 'id': 'p-hidden', + 'name': null, + 'status': null, + 'restricted': true, + }; + const readableJson = { + 'id': 'p-open', + 'name': 'Outage umbrella', + 'status': 'Pending', + 'restricted': false, + }; + + group('TicketParentSummary.fromJson', () { + test('a hidden parent reads as the tree phrase, with no status', () { + final p = TicketParentSummary.fromJson(hiddenJson); + expect(p.id, 'p-hidden'); + expect(p.restricted, isTrue); + expect(p.name, 'A ticket you cannot open'); + expect(p.name, TicketTreeNode.restrictedName); + expect(p.status, isNull); + }); + + test('a readable parent keeps its name and status', () { + final p = TicketParentSummary.fromJson(readableJson); + expect(p.restricted, isFalse); + expect(p.name, 'Outage umbrella'); + expect(p.status, 'Pending'); + }); + }); + + void usePhone(WidgetTester tester, {double textScale = 1.0}) { + tester.view.devicePixelRatio = 3.0; + tester.view.physicalSize = const Size(390 * 3, 844 * 3); + tester.platformDispatcher.textScaleFactorTestValue = textScale; + addTearDown(tester.view.resetPhysicalSize); + addTearDown(tester.view.resetDevicePixelRatio); + addTearDown(tester.platformDispatcher.clearTextScaleFactorTestValue); + } + + Ticket child(Map parent) => Ticket.fromJson({ + 'id': 'c1', + 'name': 'My sub-ticket', + 'status': 'New', + 'priority': 'Normal', + 'created_at': '2026-09-01T09:00:00Z', + 'parent': parent['id'], + 'parent_summary': parent, + }); + + group('TicketCard at 390px', () { + testWidgets('a hidden parent shows the sub-ticket pill and no name', ( + tester, + ) async { + // At 1.0: the card overflows at 1.3 with or without a parent, which is + // older than this change and not what it tests. + usePhone(tester); + await tester.pumpWidget( + MaterialApp( + theme: AppTheme.light, + home: Scaffold(body: TicketCard(ticketItem: child(hiddenJson))), + ), + ); + expect(tester.takeException(), isNull); + expect(find.text('Sub-ticket'), findsOneWidget); + }); + }); + + group('TicketDetailScreen at 390px', () { + Widget app(Ticket ticket) => ProviderScope( + overrides: [ + ticketsProvider.overrideWith(() => _FakeTicketsNotifier(ticket)), + usersProvider.overrideWithValue(const []), + tagsProvider.overrideWithValue(const []), + accountOptionsProvider.overrideWithValue(const []), + ], + child: MaterialApp.router( + theme: AppTheme.light, + routerConfig: GoRouter( + initialLocation: '/here', + routes: [ + GoRoute( + path: '/here', + builder: (_, _) => TicketDetailScreen(ticketId: ticket.id), + ), + GoRoute( + path: '/tickets/:id', + builder: (_, s) => + Scaffold(body: Text('opened ${s.pathParameters['id']}')), + ), + ], + ), + ), + ); + + testWidgets('a hidden parent is named as one and does not open', ( + tester, + ) async { + usePhone(tester); + await tester.pumpWidget(app(child(hiddenJson))); + await tester.pumpAndSettle(); + expect(tester.takeException(), isNull); + + final row = find.text('A ticket you cannot open'); + await tester.ensureVisible(row); + await tester.pumpAndSettle(); + expect(row, findsOneWidget); + + await tester.tap(row); + await tester.pumpAndSettle(); + expect(find.text('opened p-hidden'), findsNothing); + }); + + testWidgets('a readable parent is named and opens', (tester) async { + usePhone(tester); + await tester.pumpWidget(app(child(readableJson))); + await tester.pumpAndSettle(); + + final row = find.text('Outage umbrella'); + await tester.ensureVisible(row); + await tester.pumpAndSettle(); + await tester.tap(row); + await tester.pumpAndSettle(); + expect(find.text('opened p-open'), findsOneWidget); + }); + + Future openParentSheet(WidgetTester tester) async { + await tester.tap(find.byIcon(LucideIcons.moreVertical)); + await tester.pumpAndSettle(); + await tester.tap(find.text('Change / detach parent')); + await tester.pumpAndSettle(); + } + + testWidgets('detaching from a hidden parent is offered, unnamed', ( + tester, + ) async { + usePhone(tester); + await tester.pumpWidget(app(child(hiddenJson))); + await tester.pumpAndSettle(); + await openParentSheet(tester); + + expect(find.text('Detach from parent ticket'), findsOneWidget); + expect(find.textContaining('Detach from "'), findsNothing); + expect(tester.takeException(), isNull); + }); + + testWidgets('detaching from a readable parent names it', (tester) async { + usePhone(tester); + await tester.pumpWidget(app(child(readableJson))); + await tester.pumpAndSettle(); + await openParentSheet(tester); + + expect(find.text('Detach from "Outage umbrella"'), findsOneWidget); + }); + + testWidgets('a long parent name wraps in the sheet at large text', ( + tester, + ) async { + usePhone(tester, textScale: 1.3); + await tester.pumpWidget( + app( + child({ + ...readableJson, + 'name': 'Every printer on the third floor stopped at once', + }), + ), + ); + await tester.pumpAndSettle(); + await openParentSheet(tester); + + expect(find.textContaining('Every printer'), findsWidgets); + expect(tester.takeException(), isNull); + }); + }); +} + +class _FakeTicketsNotifier extends TicketsNotifier { + _FakeTicketsNotifier(this.ticket); + + final Ticket ticket; + + @override + Future build() async => + TicketsListData(tickets: const [], totalCount: 0, hasMore: false); + + @override + Future getTicketDetail(String id) async => + TicketDetailResult( + ticketObj: ticket, + activities: const [], + commentPermission: true, + internalCommentIds: const {}, + ); + + @override + Future getWatchers(String id) async => const TicketWatchers( + watchers: [], + count: 0, + isCurrentUserWatching: false, + ); + + @override + Future fetchTree(String id) async => null; +} diff --git a/mobile/test/unit/ticket_tree_close_test.dart b/mobile/test/unit/ticket_tree_close_test.dart index cf8805c2..d9cb6dc7 100644 --- a/mobile/test/unit/ticket_tree_close_test.dart +++ b/mobile/test/unit/ticket_tree_close_test.dart @@ -100,6 +100,31 @@ void main() { expect(merged.openDescendantsOf('p').map((n) => n.id), ['real']); }); + test('lists a ticket the viewer cannot open, without a name', () { + // `/tree/` redacts it to an id, a status and `restricted`. It still + // counts, because the close refuses a cascade that would take it. + final redacted = TicketTreeNode.fromJson( + treeJson( + id: 'p', + children: [ + { + 'id': 'hidden', + 'name': null, + 'restricted': true, + 'status': 'New', + 'is_active': true, + 'children': const [], + }, + ], + ), + ); + final open = redacted.openDescendantsOf('p'); + expect(open.map((n) => n.id), ['hidden']); + expect(open.single.restricted, isTrue); + expect(open.single.name, TicketTreeNode.restrictedName); + expect(redacted.restricted, isFalse); + }); + test('is empty for a leaf, and for a ticket not in the tree', () { expect(tree.openDescendantsOf('kidOpen'), isEmpty); expect(tree.openDescendantsOf('nope'), isEmpty);