Add unit tests for organization admin checks and saved views provider

- Implement tests for `isOrgAdminProvider` to validate admin roles based on organization membership and role.
- Create tests for `saved_views_provider` to ensure correct API interactions for saving, renaming, and deleting saved views.
- Enhance ticket tree tests to accurately reflect inactive and merged ticket handling.
- Update CSV import tests to verify error handling and file saving functionality.
- Introduce tests for deal cards to ensure correct display of next steps and task statuses.
This commit is contained in:
Ashwin 2026-09-26 20:38:32 +05:30
parent ca939e6dfe
commit dd6ca2958d
365 changed files with 18884 additions and 2879 deletions

View file

@ -53,6 +53,17 @@ CSRF_TRUSTED_ORIGINS=http://localhost:5181
# or the header becomes something any caller can forge.
TRUST_PROXY_SSL_HEADER=False
# Reverse proxies in front of Django that append to X-Forwarded-For (nginx with
# $proxy_add_x_forwarded_for is one). Every recorded client IP (audit log, web
# form submissions, estimate acceptance, sign-in tokens) and every per-IP
# throttle trusts only that many entries from the right; unset uses REMOTE_ADDR.
# Leave it unset if the app server already resolves the client from trusted
# proxies (uvicorn --proxy-headers --forwarded-allow-ips=127.0.0.1). Set it to 1
# when REMOTE_ADDR is the proxy itself (gunicorn behind one nginx), or every
# visitor shares the proxy's throttle bucket. Never set it on top of uvicorn's
# proxy handling: SvelteKit-relayed requests would then all count as the relay.
NUM_PROXIES=
# Shared cache. Required in production for DRF throttling to be enforced across
# workers rather than per process. Without it the public web form rate limits
# are per gunicorn worker and reset on restart.

View file

@ -1,18 +1,15 @@
"""Who may open an account.
Extracted from ``AccountDetailView.assert_account_access`` so the attachment
download view asks the same question rather than carrying a second copy of the
answer. The detail view still calls it.
One definition, asked by the detail and mail views (through
``visible_accounts_qs``, in the lookup itself, so a hidden account is the same
404 as a missing one) and by the attachment download (``has_account_access``).
"""
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"
def has_account_access(profile, user, account):
"""Admins, superusers, the creator, and anyone assigned. Else refused.
@ -45,9 +42,3 @@ def visible_accounts_qs(profile, user):
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):
raise PermissionDenied(_DENIED)

View file

@ -119,7 +119,7 @@ class TestAccountDetailAccess:
def test_unrelated_non_admin_is_refused(self, user_client, account, admin_user):
_created_by(account, admin_user)
response = user_client.get(f"/api/accounts/{account.id}/")
assert response.status_code == 403
assert response.status_code == 404
def test_another_org_gets_404_not_403(self, org_b_client, account):
"""Wrong tenant is 'no such account', not 'not allowed'.
@ -139,7 +139,7 @@ class TestAccountDetailAccess:
"""
assert account.created_by is None
response = user_client.get(f"/api/accounts/{account.id}/")
assert response.status_code == 403
assert response.status_code == 404
@pytest.mark.django_db
@ -194,7 +194,7 @@ class TestAccountVerbsAgree:
{"city": "Nowhere"},
content_type="application/json",
)
assert response.status_code == 403
assert response.status_code == 404
account.refresh_from_db()
assert account.city is None
@ -205,7 +205,7 @@ class TestAccountVerbsAgree:
{"name": "Renamed By A Stranger"},
content_type="application/json",
)
assert response.status_code == 403
assert response.status_code == 404
account.refresh_from_db()
assert account.name == "Northwind Traders"
@ -222,7 +222,7 @@ class TestAccountVerbsAgree:
{"name": ""},
content_type="application/json",
)
assert response.status_code == 403
assert response.status_code == 404
def test_comment_on_missing_account_is_404(self, admin_client):
response = admin_client.post(
@ -236,7 +236,7 @@ class TestAccountVerbsAgree:
response = user_client.post(
f"/api/accounts/{account.id}/", {"comment": "hello"}
)
assert response.status_code == 403
assert response.status_code == 404
@pytest.mark.django_db
@ -283,6 +283,7 @@ class TestAttachmentDeleteIsScoped:
was quietly admin-only.
"""
mine = self._attachment(org_a, account)
account.assigned_to.add(user_profile) # can open it
Attachments.objects.filter(pk=mine.pk).update(created_by=user_profile.user)
response = user_client.delete(f"/api/accounts/attachment/{mine.id}/")
@ -291,9 +292,10 @@ class TestAttachmentDeleteIsScoped:
assert not Attachments.objects.filter(pk=mine.pk).exists()
def test_other_users_attachment_is_refused(
self, user_client, admin_user, org_a, account
self, user_profile, user_client, admin_user, org_a, account
):
theirs = self._attachment(org_a, account)
account.assigned_to.add(user_profile) # can open it
Attachments.objects.filter(pk=theirs.pk).update(created_by=admin_user)
response = user_client.delete(f"/api/accounts/attachment/{theirs.id}/")
@ -774,13 +776,13 @@ class TestSidePayloadsRespectRole:
"""The two doors must agree.
This is the actual defect: the same lead id, refused by its own
endpoint and handed over by this one. Asserting the 403 here rather
endpoint and handed over by this one. Asserting the 404 here rather
than only the absence keeps the test honest if the detail rule moves.
"""
stranger = self._lead(org_a, "Stranger")
Lead.objects.filter(pk=stranger.pk).update(created_by=admin_user)
assert user_client.get(f"/api/leads/{stranger.id}/").status_code == 403
assert user_client.get(f"/api/leads/{stranger.id}/").status_code == 404
payload = user_client.get("/api/accounts/").json()
assert str(stranger.id) not in {row["id"] for row in payload["leads"]}

View file

@ -110,7 +110,8 @@ def test_hidden_records_are_refused_by_their_own_endpoints(user_client, account)
module's own detail route refuses too."""
hidden_deal = Opportunity.objects.get(name="Hidden deal")
hidden_task = Task.objects.get(title="Hidden task")
assert user_client.get(f"/api/opportunities/{hidden_deal.pk}/").status_code == 403
# 404 for a deal: a hidden one answers as a missing one (1.11.0).
assert user_client.get(f"/api/opportunities/{hidden_deal.pk}/").status_code == 404
assert user_client.get(f"/api/tasks/{hidden_task.pk}/").status_code == 403

View file

@ -64,7 +64,7 @@ class TestDetailListsVisibleContacts:
"""The filter mirrors the contact endpoint's own rule, not a new one."""
hidden = user_client.get(f"/api/contacts/{world['hidden'].id}/")
mine = user_client.get(f"/api/contacts/{world['mine'].id}/")
assert hidden.status_code == 403
assert hidden.status_code == 404
assert mine.status_code == 200
def test_admin_sees_every_contact(self, admin_client, world):

View file

@ -0,0 +1,149 @@
"""An account the caller may not open answers exactly like one that does not exist.
Owner decision for 1.11.0: the account detail, mail, comment and attachment
endpoints look the account up through `accounts.access.visible_accounts_qs`
in one step. They used to fetch org-wide and then check, answering 403 for a
hidden account and 404 for a missing one. Each test compares the two
responses WHOLE. Writes narrower than reads keep their 403, only for a caller
who can open the account.
"""
from unittest import mock
import pytest
from django.contrib.contenttypes.models import ContentType
from accounts.models import Account
from common.models import Attachments, Comment
MISSING = "11111111-1111-1111-1111-111111111111"
@pytest.fixture
def account(org_a):
"""In the caller's org, created by nobody, assigned to nobody: hidden."""
return Account.objects.create(name="Hidden Co", org=org_a)
@pytest.fixture
def comment(account, admin_profile, org_a):
return Comment.objects.create(
content_type=ContentType.objects.get_for_model(Account),
object_id=account.id,
comment="Not yours",
commented_by=admin_profile,
org=org_a,
)
@pytest.fixture
def attachment(account, admin_user, org_a):
return Attachments.objects.create(
content_type=ContentType.objects.get_for_model(Account),
object_id=account.id,
file_name="x.txt",
attachment="attachments/x.txt",
created_by=admin_user,
org=org_a,
)
def _same(client, method, hidden_url, missing_url, body=None):
call = getattr(client, method)
kwargs = {"format": "json"} if body is not None else {}
args = (body,) if body is not None else ()
hidden = call(hidden_url, *args, **kwargs)
missing = call(missing_url, *args, **kwargs)
assert hidden.status_code == missing.status_code == 404
assert hidden.json() == missing.json()
@pytest.mark.django_db
class TestAccountIdEndpoints:
@pytest.mark.parametrize(
"method,body",
[
("get", None),
("put", {"name": "Mine now"}),
("patch", {"name": "Mine now"}),
("post", {"comment": "hello"}),
("delete", None),
],
)
def test_detail(self, user_client, account, method, body):
_same(
user_client,
method,
f"/api/accounts/{account.id}/",
f"/api/accounts/{MISSING}/",
body,
)
account.refresh_from_db()
assert account.name == "Hidden Co"
def test_create_mail(self, user_client, account):
body = {
"from_email": "sales@example.com",
"message_subject": "Hello",
"message_body": "Body",
}
with mock.patch("accounts.views.send_email.delay") as dispatch:
_same(
user_client,
"post",
f"/api/accounts/{account.id}/create_mail/",
f"/api/accounts/{MISSING}/create_mail/",
body,
)
dispatch.assert_not_called()
@pytest.mark.django_db
class TestCommentsAndFiles:
@pytest.mark.parametrize(
"method,body",
[("put", {"comment": "x"}), ("patch", {"comment": "x"}), ("delete", None)],
)
def test_comment_on_hidden_account(self, user_client, comment, method, body):
_same(
user_client,
method,
f"/api/accounts/comment/{comment.id}/",
f"/api/accounts/comment/{MISSING}/",
body,
)
assert Comment.objects.filter(id=comment.id).exists()
def test_attachment_on_hidden_account(self, user_client, attachment):
_same(
user_client,
"delete",
f"/api/accounts/attachment/{attachment.id}/",
f"/api/accounts/attachment/{MISSING}/",
)
assert Attachments.objects.filter(id=attachment.id).exists()
def test_readable_account_non_author_is_403(
self, user_client, user_profile, account, comment, attachment
):
account.assigned_to.add(user_profile)
url = f"/api/accounts/comment/{comment.id}/"
assert user_client.put(url, {"comment": "x"}, format="json").status_code == 403
assert user_client.delete(url).status_code == 403
attachment_url = f"/api/accounts/attachment/{attachment.id}/"
assert user_client.delete(attachment_url).status_code == 403
@pytest.mark.django_db
class TestDeleteStaysNarrower:
def test_assignee_who_is_not_creator_gets_403(
self, user_client, user_profile, account
):
account.assigned_to.add(user_profile)
assert user_client.get(f"/api/accounts/{account.id}/").status_code == 200
assert user_client.delete(f"/api/accounts/{account.id}/").status_code == 403
assert Account.objects.filter(id=account.id).exists()
def test_creator_may_delete(self, user_client, regular_user, account):
Account.objects.filter(id=account.id).update(created_by=regular_user)
assert user_client.delete(f"/api/accounts/{account.id}/").status_code == 200

View file

@ -47,7 +47,7 @@ class TestSuperuserAccountAccess:
assert has_account_access(user_profile, superuser, admins_account) is True
def test_plain_member_is_refused_the_detail(self, user_client, admins_account):
assert user_client.get(f"/api/accounts/{admins_account.id}/").status_code == 403
assert user_client.get(f"/api/accounts/{admins_account.id}/").status_code == 404
def test_superuser_may_open_the_detail(
self, superuser, user_client, admins_account
@ -111,7 +111,7 @@ class TestLeadCatalogue:
}
def test_plain_member_does_not_get_a_lead_they_cannot_open(self, user_client, lead):
assert user_client.get(f"/api/leads/{lead.id}/").status_code == 403
assert user_client.get(f"/api/leads/{lead.id}/").status_code == 404
assert user_client.get("/api/accounts/").json()["leads"] == []
def test_superuser_gets_the_lead_their_detail_route_serves(

View file

@ -388,17 +388,17 @@ class TestAccountDetailView:
name="Not My Account", org=org_a, created_by=admin_user
)
response = user_client.delete(f"/api/accounts/{account.id}/")
assert response.status_code == 403
assert response.status_code == 404
def test_get_account_non_admin_not_assigned(
self, user_client, org_a, admin_user, user_profile
):
"""Non-admin user not assigned to account gets 403."""
"""Non-admin user not assigned to account gets 404, as for a missing id."""
account = Account.objects.create(
name="Private Account", org=org_a, created_by=admin_user
)
response = user_client.get(f"/api/accounts/{account.id}/")
assert response.status_code == 403
assert response.status_code == 404
def test_get_account_non_admin_assigned(
self, user_client, org_a, admin_user, user_profile
@ -423,7 +423,7 @@ class TestAccountDetailView:
{"name": "Not Assigned"},
format="json",
)
assert response.status_code == 403
assert response.status_code == 404
def test_patch_account(self, admin_client, org_a):
"""Test partial update via PATCH."""
@ -478,7 +478,7 @@ class TestAccountDetailView:
assert account.assigned_to.count() == 0
def test_patch_account_non_admin_forbidden(self, user_client, org_a, admin_user):
"""Non-admin user not assigned gets 403 on PATCH."""
"""Non-admin user not assigned gets 404 on PATCH, as on GET."""
account = Account.objects.create(
name="Patch Forbidden", org=org_a, created_by=admin_user
)
@ -487,7 +487,7 @@ class TestAccountDetailView:
{"city": "Nope"},
format="json",
)
assert response.status_code == 403
assert response.status_code == 404
def test_add_comment_via_post(self, admin_client, org_a, admin_profile):
"""Test adding a comment to an account via POST on account detail."""
@ -519,7 +519,7 @@ class TestAccountDetailView:
{"comment": "Not allowed"},
format="json",
)
assert response.status_code == 403
assert response.status_code == 404
@pytest.mark.django_db
@ -559,6 +559,8 @@ class TestAccountCommentView:
):
"""Non-admin user who didn't create the comment cannot edit."""
account = Account.objects.create(name="Comment Perm Account", org=org_a)
# Can open the account, so the author check is what answers.
account.assigned_to.add(user_profile)
comment = self._create_comment(account, admin_profile, org_a)
response = user_client.put(
f"/api/accounts/comment/{comment.id}/",
@ -572,6 +574,8 @@ class TestAccountCommentView:
):
"""Non-admin user who didn't create the comment cannot delete."""
account = Account.objects.create(name="Comment Del Perm", org=org_a)
# Can open the account, so the author check is what answers.
account.assigned_to.add(user_profile)
comment = self._create_comment(account, admin_profile, org_a)
response = user_client.delete(f"/api/accounts/comment/{comment.id}/")
assert response.status_code == 403
@ -619,6 +623,8 @@ class TestAccountCommentView:
):
"""Authorization is still checked before the body is."""
account = Account.objects.create(name="Empty Body Perm", org=org_a)
# Can open the account, so the author check is what answers.
account.assigned_to.add(user_profile)
comment = self._create_comment(account, admin_profile, org_a)
response = user_client.put(
f"/api/accounts/comment/{comment.id}/",
@ -644,6 +650,8 @@ class TestAccountCommentView:
):
"""Non-admin user who didn't create the comment cannot PATCH."""
account = Account.objects.create(name="Patch Perm Account", org=org_a)
# Can open the account, so the author check is what answers.
account.assigned_to.add(user_profile)
comment = self._create_comment(account, admin_profile, org_a)
response = user_client.patch(
f"/api/accounts/comment/{comment.id}/",
@ -655,6 +663,8 @@ class TestAccountCommentView:
def test_own_comment_can_edit(self, user_client, org_a, user_profile):
"""User who created the comment can edit it."""
account = Account.objects.create(name="Own Comment Account", org=org_a)
# Can open the account, so the author check is what answers.
account.assigned_to.add(user_profile)
comment = self._create_comment(account, user_profile, org_a)
response = user_client.put(
f"/api/accounts/comment/{comment.id}/",
@ -666,6 +676,8 @@ class TestAccountCommentView:
def test_own_comment_can_delete(self, user_client, org_a, user_profile):
"""User who created the comment can delete it."""
account = Account.objects.create(name="Own Del Comment Account", org=org_a)
# Can open the account, so the author check is what answers.
account.assigned_to.add(user_profile)
comment = self._create_comment(account, user_profile, org_a)
response = user_client.delete(f"/api/accounts/comment/{comment.id}/")
assert response.status_code == 200
@ -697,6 +709,8 @@ class TestAccountAttachmentView:
):
"""Non-admin who didn't create the attachment cannot delete."""
account = Account.objects.create(name="Att Perm Account", org=org_a)
# Can open the account, so the author check is what answers.
account.assigned_to.add(user_profile)
attachment = self._create_attachment(account, admin_profile.user, org_a)
response = user_client.delete(f"/api/accounts/attachment/{attachment.id}/")
assert response.status_code == 403

View file

@ -183,11 +183,15 @@ class TestCreateMailAccountAccess:
def test_member_who_cannot_open_the_account_is_refused(
self, user_client, admins_account
):
"""403, matching `GET` on the same account, and no mail row left."""
assert user_client.get(f"/api/accounts/{admins_account.id}/").status_code == 403
"""404, matching `GET` on the same account, and no mail row left."""
assert user_client.get(f"/api/accounts/{admins_account.id}/").status_code == 404
with mock.patch("accounts.views.send_email.delay") as dispatch:
resp = user_client.post(_url(admins_account.id), _payload())
assert resp.status_code == 403
assert resp.status_code == 404
missing = user_client.post(
_url("11111111-1111-1111-1111-111111111111"), _payload()
)
assert resp.json() == missing.json()
assert not AccountEmail.objects.filter(from_account=admins_account).exists()
dispatch.assert_not_called()

View file

@ -81,7 +81,7 @@ class TestNestedAccountOverTheApi:
def test_case_viewer_without_account_access_gets_id_and_name(
self, user_client, account, case
):
assert user_client.get(f"/api/accounts/{account.id}/").status_code == 403
assert user_client.get(f"/api/accounts/{account.id}/").status_code == 404
response = user_client.get(f"/api/cases/{case.id}/")
assert response.status_code == 200, response.content
_only_id_and_name(response.json()["cases_obj"]["account"], account)

View file

@ -2,7 +2,6 @@ import json
import uuid
from django.contrib.contenttypes.models import ContentType
from django.core.exceptions import ValidationError as DjangoValidationError
from django.db.models import (
Count,
DateField,
@ -16,7 +15,6 @@ from django.db.models import (
)
from django.db.models.deletion import ProtectedError
from django.db.models.functions import Coalesce
from django.http import Http404
from django.shortcuts import get_object_or_404
from django.utils import timezone
from drf_spectacular.utils import extend_schema
@ -44,7 +42,7 @@ from cases.models import Case
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
from common.lookups import get_on_visible_record_or_404
from common.models import (
Attachments,
Comment,
@ -509,30 +507,22 @@ class AccountDetailView(APIView):
serializer_class = AccountSerializer
def get_object(self, pk):
# A malformed id is 404, not 500. The URL pattern is `<str:pk>`, so an
# old bookmark or a typo reaches the UUID field as text, and
# `UUIDField.to_python` raises `ValidationError`, which
# `get_object_or_404` does not catch, because it only knows about
# `DoesNotExist`. "That is not an id" and "no such account" are the
# same answer to whoever asked.
try:
account = get_object_or_404(
annotate_rollups(Account.objects.all(), self.request.profile),
id=pk,
org=self.request.profile.org,
)
except (DjangoValidationError, ValueError):
raise Http404("No such account.")
attach_money_rollups([account], self.request.profile)
return account
"""The account, or 404 when it is missing or the caller may not open it.
def assert_account_access(self, account):
"""Delegates to `accounts.access`, which holds the one definition.
The attachment download view asks the same question, and four inline
copies is how the creator branch came to be dead in all four verbs.
Looked up through the read rule (`accounts.access.visible_accounts_qs`)
in the same query, so a same-org account the caller cannot open
answers every verb exactly as an id that does not exist. Fetching
org-wide and checking afterwards answered 403, which confirmed it.
"""
access.assert_account_access(self.request.profile, self.request.user, account)
profile = self.request.profile
account = get_object_or_404(
annotate_rollups(Account.objects.all(), profile),
id=pk,
org=profile.org,
id__in=access.visible_accounts_qs(profile, self.request.user).values("id"),
)
attach_money_rollups([account], profile)
return account
@extend_schema(
tags=["Accounts"],
@ -543,10 +533,6 @@ class AccountDetailView(APIView):
def put(self, request, pk, format=None):
data = request.data
account_object = self.get_object(pk=pk)
# Authorise before validating. The check used to sit inside
# `is_valid()`, so somebody with no right to touch the account learned
# whether their payload was well-formed before being turned away.
self.assert_account_access(account_object)
# Parsed before the first write: a malformed id used to be a 400 after
# the fields were saved and the contacts replaced.
contact_ids = payload_id_list(data.get("contacts"), "contacts")
@ -643,12 +629,10 @@ class AccountDetailView(APIView):
def delete(self, request, pk, format=None):
self.object = self.get_object(pk)
# Admins, superusers and the creator: the deal delete rule. Assignees
# may open and edit an account but not delete it. Superusers were
# refused here alone, while every sibling rule let them through.
if (
not is_org_admin(self.request.profile)
and not self.request.user.is_superuser
):
# may open and edit an account but not delete it, and since they can
# open it, that refusal is an honest 403. Superusers were refused here
# alone, while every sibling rule let them through.
if not is_org_admin(self.request.profile):
if self.request.profile.user_id != self.object.created_by_id:
return Response(
{
@ -684,7 +668,6 @@ class AccountDetailView(APIView):
)
def get(self, request, pk, format=None):
self.account = self.get_object(pk=pk)
self.assert_account_access(self.account)
context = {}
# Every related list below applies its own module's read rule, the one
# that module's detail endpoint enforces. Being able to open the
@ -808,7 +791,6 @@ class AccountDetailView(APIView):
# simply is not there, while GET on the same id answered 404. Commenting
# on a deleted account is a normal race, not a server fault.
self.account_obj = self.get_object(pk=pk)
self.assert_account_access(self.account_obj)
# Before the comment is saved, so a refused file does not leave it posted.
validate_attachment(self.request.FILES.get("account_attachment"))
# This block never created a comment. `object_id` and `org` were
@ -872,7 +854,6 @@ class AccountDetailView(APIView):
"""Handle partial updates to an account."""
data = request.data
account_object = self.get_object(pk=pk)
self.assert_account_access(account_object)
# Parsed before the first write, as in PUT. An absent key parses to [].
contact_ids = payload_id_list(data.get("contacts"), "contacts")
tag_ids = payload_id_list(data.get("tags"), "tags")
@ -965,7 +946,14 @@ class AccountCommentView(APIView):
serializer_class = AccountCommentEditSwaggerSerializer
def get_object(self, pk):
return get_scoped_or_404(self.model, pk, self.request.profile.org)
# 404, like a missing id, for a comment on an account the caller
# cannot open.
return get_on_visible_record_or_404(
self.model,
pk,
self.request.profile.org,
access.visible_accounts_qs(self.request.profile, self.request.user),
)
@extend_schema(
tags=["Accounts"],
@ -1068,12 +1056,15 @@ class AccountAttachmentView(APIView):
# `created_by` is a User FK, so the ownership branch compares against
# `profile.user_id`; comparing the Profile itself was never true, which
# made this admin-only by accident.
try:
self.object = get_object_or_404(
self.model, pk=pk, org=self.request.profile.org
)
except (DjangoValidationError, ValueError):
raise Http404("No such attachment.")
#
# A file on an account the caller cannot open is the same 404 as a
# missing id.
self.object = get_on_visible_record_or_404(
self.model,
pk,
request.profile.org,
access.visible_accounts_qs(request.profile, request.user),
)
if (
is_org_admin(request.profile)
or request.profile.user_id == self.object.created_by_id
@ -1130,18 +1121,22 @@ class AccountCreateMailView(APIView):
name cannot be put on mail to anyone else in the org.
Within the org, only someone who may open the account may send from
it. Anyone else is refused with the 403 that `GET` on the same account
gives them, so the two verbs agree on what the caller may know.
it. Anyone else gets the 404 that `GET` on the same account gives
them, identical to an id that does not exist, so the two verbs agree
on what the caller may know.
"""
params = request.data
scheduled_date_time = params.get("scheduled_date_time")
account = Account.objects.filter(id=pk, org=request.profile.org).first()
account = (
access.visible_accounts_qs(request.profile, request.user)
.filter(id=pk)
.first()
)
if account is None:
return Response(
{"error": True, "errors": "Account not found"},
status=status.HTTP_404_NOT_FOUND,
)
access.assert_account_access(request.profile, request.user, account)
serializer = EmailSerializer(data=params, request_obj=request)
if not serializer.is_valid():

View file

@ -45,6 +45,10 @@ _DENIED = "You do not have Permission to perform this action"
def visible_cases_qs(profile):
"""Cases ``profile`` is allowed to open. The queryset form of `read`.
An org admin sees every case in the org, and `is_org_admin` counts a
superuser's profile as one, as the contact, lead, account and deal
helpers do. It used to be the one read rule that ignored superusers.
The watcher clause is what lets somebody keep following a ticket after
they are unassigned, and it is the clause the detail view was missing.
"""
@ -69,8 +73,17 @@ def writable_cases_qs(profile):
return qs.filter(Q(created_by=profile.user) | Q(assigned_to=profile)).distinct()
_NOT_FOUND = "No such case."
def get_case_or_404(profile, pk):
"""Fetch a case in the requester's org, or raise ``Http404``.
"""A case ``profile`` may open, or ``Http404``.
Looked up through `visible_cases_qs`, so a ticket in the caller's org that
they may not open answers exactly like one that does not exist: same
status, same body. Answering 403 for it confirmed the ticket existed. A
caller who may open the ticket but not change it still gets 403 from
`assert_case_write_access`; knowing it exists is theirs already.
``Case.id`` is a UUID column, so ``filter(id="nobody")`` raises Django's
``ValidationError`` rather than returning nothing, which surfaced as a
@ -78,11 +91,27 @@ def get_case_or_404(profile, pk):
a request for a case that does not exist; that is a 404.
"""
try:
case = Case.objects.filter(pk=pk, org=profile.org).first()
case = visible_cases_qs(profile).filter(pk=pk).first()
except (DjangoValidationError, ValueError):
raise Http404("No such case.")
raise Http404(_NOT_FOUND)
if case is None:
raise Http404("No such case.")
raise Http404(_NOT_FOUND)
return case
def lock_case_or_404(profile, pk):
"""`get_case_or_404` with the row locked for the caller's transaction.
Postgres refuses ``SELECT ... FOR UPDATE`` with ``DISTINCT``, which
`visible_cases_qs` adds for a non-admin, so the row is locked by id and
the read rule is asked of it after. The answer is the same 404.
"""
try:
case = Case.objects.select_for_update().filter(pk=pk, org=profile.org).first()
except (DjangoValidationError, ValueError):
raise Http404(_NOT_FOUND)
if case is None or not has_case_read_access(profile, case):
raise Http404(_NOT_FOUND)
return case
@ -102,14 +131,12 @@ def has_case_write_access(profile, case):
return profile.id in {p.id for p in case.assigned_to.all()}
def assert_case_read_access(profile, case):
"""Raise 403 unless ``profile`` may open ``case``."""
if not has_case_read_access(profile, case):
raise PermissionDenied(_DENIED)
def assert_case_write_access(profile, case):
"""Raise 403 unless ``profile`` may change ``case`` or reply on it."""
"""Raise 403 unless ``profile`` may change ``case`` or reply on it.
Call it on a case from `get_case_or_404`, so a caller who may not even
open the case has already had the 404.
"""
if not has_case_write_access(profile, case):
raise PermissionDenied(_DENIED)

View file

@ -14,7 +14,6 @@ from datetime import datetime, timedelta
from typing import Optional
from uuid import UUID
from django.db.models import Q
from django.utils import timezone
from drf_spectacular.utils import OpenApiParameter, extend_schema
from rest_framework.exceptions import ValidationError
@ -23,7 +22,7 @@ from rest_framework.response import Response
from rest_framework.views import APIView
from cases import analytics
from cases.access import is_org_admin
from cases.access import is_org_admin, visible_cases_qs
from cases.analytics import DEFAULT_SERVICE_DAYS
from cases.models import Case
from cases.serializer import CaseSerializer, parent_access_context
@ -95,23 +94,19 @@ def _parse_days(value) -> int:
def _filtered_qs(request) -> tuple[object, datetime, datetime]:
"""Build the org-scoped Case queryset + (from, to) window from query params.
Visibility: admins see every case; non-admins see only those they
created, are assigned to, or watch. Mirrors `CaseListView.get_queryset`.
Visibility is `visible_cases_qs`, the ticket read rule: admins and
superusers see every case; everyone else only those they created, are
assigned to, or watch. This used to carry its own copy of that rule.
"""
profile = request.profile
params = request.query_params
qs = (
Case.objects.filter(org=profile.org, is_active=True)
.filter(merged_into__isnull=True)
visible_cases_qs(profile)
.filter(is_active=True, merged_into__isnull=True)
.exclude(status="Duplicate")
)
if not is_org_admin(profile):
qs = qs.filter(
Q(created_by=profile.user) | Q(assigned_to=profile) | Q(watchers=profile)
).distinct()
if priority := params.get("priority"):
qs = qs.filter(priority=priority)
if team_id := uuid_param(params, "team"):
@ -216,9 +211,11 @@ class AnalyticsServiceView(_AnalyticsBaseView):
{"error": True, "errors": "Admin access required"}, status=403
)
days = _parse_days(request.query_params.get("days"))
qs = Case.objects.filter(
org=request.profile.org, is_active=True, merged_into__isnull=True
).exclude(status="Duplicate")
qs = (
visible_cases_qs(request.profile)
.filter(is_active=True, merged_into__isnull=True)
.exclude(status="Duplicate")
)
data = analytics.compute_service_overview(qs, request.profile.org_id, days)
return Response(data)

View file

@ -18,16 +18,18 @@ from __future__ import annotations
from django.db import transaction
from django.db.models import Count
from django.shortcuts import get_object_or_404
from django.utils import timezone
from rest_framework import 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, has_case_read_access
from cases.access import (
assert_case_write_access,
get_case_or_404,
has_case_read_access,
)
from cases.approvals import Approval, ApprovalRule, find_matching_rule
from cases.models import Case
from cases.serializer import (
ApprovalRequestSerializer,
ApprovalRuleSerializer,
@ -238,7 +240,8 @@ class CaseRequestApprovalView(APIView):
@transaction.atomic
def post(self, request, pk):
org = request.profile.org
case = get_object_or_404(Case, id=pk, org=org)
# A case the caller may not open is a 404, as on the detail view.
case = get_case_or_404(request.profile, pk)
# Filing an approval acts on the case and returns its summary, so it
# needs the same write access as replying on it. Org membership alone
# let a member who cannot open a case create a row against it and read

View file

@ -20,6 +20,7 @@ from django.db import models
from common.base import BaseModel
from common.models import Org, Profile, Teams
from common.permissions import is_org_admin
from common.utils import CASE_TYPE, PRIORITY_CHOICE
# Approver roles. Mirrors the spec's ``ADMIN``/``MANAGER`` choices even though
@ -227,6 +228,10 @@ class Approval(BaseModel):
return False
if self.rule.approvers.filter(id=profile.id).exists():
return True
if self.rule.approver_role == "ADMIN":
# "Admin" means `is_org_admin`, which also admits a superuser's
# profile, not a literal read of `role`.
return is_org_admin(profile)
if self.rule.approver_role and profile.role == self.rule.approver_role:
return True
return False

View file

@ -9,9 +9,9 @@ from rest_framework.permissions import IsAuthenticated
from rest_framework.response import Response
from rest_framework.views import APIView
from cases.access import has_case_write_access, is_org_admin
from cases.access import has_case_write_access, is_org_admin, visible_cases_qs
from cases.models import Case
from cases.workflow import DUPLICATE_BY_MERGE_ONLY
from cases.workflow import DUPLICATE_BY_MERGE_ONLY, merged_status_refusal
from common.models import Activity, Profile, Tags
from common.permissions import HasOrgContext
@ -131,16 +131,33 @@ class BulkUpdateCasesView(APIView):
org = request.profile.org
results = []
updated_count = 0
for case in Case.objects.filter(pk__in=ids, org=org):
# Only tickets the caller may open. One they may not is left out of
# `results` exactly as a missing id is, so the batch cannot be used to
# learn which ids exist.
for case in Case.objects.filter(
org=org,
pk__in=visible_cases_qs(request.profile).filter(pk__in=ids).values("pk"),
):
# Per-case authorization, mirroring the single-case PUT path
# (`CaseDetailView.put` calls `assert_case_write_access`). Without
# this any org member could edit, reassign or close any case in
# the org. Cases the caller may not write are reported as
# the org. Cases the caller may open but not write are reported as
# `no_access` rather than silently skipped, so the caller can see
# which of their selected tickets were denied.
if not has_case_write_access(request.profile, case):
results.append({"id": str(case.pk), "status": "no_access"})
continue
# The single-case rule, reported per ticket like the close gate.
refusal = merged_status_refusal(case, fields.get("status", case.status))
if refusal:
results.append(
{
"id": str(case.pk),
"status": "merged",
"detail": refusal["status"],
}
)
continue
try:
# A savepoint per case, so a blocked close rolls back only
# itself and the rest of the batch still commits.
@ -199,10 +216,13 @@ class BulkDeleteCasesView(APIView):
results = []
deletable = []
# Deleting is admin-or-creator only (`assert_case_delete_access`); an
# assignee may work a ticket but not erase it.
for row in Case.objects.filter(pk__in=ids, org=org, is_active=True).values(
"id", "name", "created_by_id"
):
# assignee may work a ticket but not erase it. A ticket the caller may
# not open is left out of `results`, as a missing id is.
for row in Case.objects.filter(
org=org,
pk__in=visible_cases_qs(request.profile).filter(pk__in=ids).values("pk"),
is_active=True,
).values("id", "name", "created_by_id"):
if admin or row["created_by_id"] == request.profile.user_id:
deletable.append(row)
else:

View file

@ -48,12 +48,13 @@ NOT_FOUND = {"error": "Not found"}
class HelpCenterIPThrottle(SimpleRateThrottle):
"""Per-visitor rate limit, bucketed on the forwarded client IP.
"""Per-visitor rate limit, bucketed on `client_ip`.
The web app renders these pages server-side and forwards the visitor's
address, so the bucket is the visitor rather than the web server. The
header is spoofable, which makes this a brake on casual scraping, not a
wall; the data behind it is public by the org's own choice.
`client_ip` believes only the `X-Forwarded-For` entries our own proxies
appended (`NUM_PROXIES`), so rotating the header no longer buys a fresh
bucket. These pages are rendered by the web app's server, which calls the
API itself: whether the bucket is the visitor or that server depends on
how many trusted hops sit between the two (see `client_ip`).
"""
scope = "help_center_ip"
@ -68,8 +69,8 @@ class HelpCenterIPThrottle(SimpleRateThrottle):
class HelpCenterGlobalThrottle(SimpleRateThrottle):
"""Per-help-center rate limit across every visitor.
The backstop for `HelpCenterIPThrottle`: rotating `X-Forwarded-For` buys a
fresh per-visitor bucket every request but cannot touch this one. Bucketed
The backstop for `HelpCenterIPThrottle`, for a caller spread across many
addresses. Bucketed
on the slug, as the web form limit is bucketed on the form, so a scraper
hammering one org's help center cannot lock readers out of another's.
"""

View file

@ -13,7 +13,11 @@ 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.access import (
assert_case_write_access,
lock_case_or_404,
visible_cases_qs,
)
from cases.approvals import close_refusal
from cases.models import Case, CasePipeline, CaseStage
from cases.serializer import (
@ -23,7 +27,7 @@ from cases.serializer import (
CasePipelineSerializer,
CaseStageSerializer,
)
from cases.workflow import duplicate_refusal
from cases.workflow import duplicate_refusal, merged_status_refusal
from common.kanban import place_in_column
from common.permissions import HasOrgContext, is_org_admin
from common.utils import STATUS_CHOICE
@ -246,13 +250,14 @@ class CaseMoveView(APIView):
"""Move case to different column and/or position."""
org = request.profile.org
# Locked for the transaction: the move saves the whole row, so an
# 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)
# edit committing between this read and that save would be lost. A
# ticket the caller may not open is a 404, as on the detail view.
case = lock_case_or_404(request.profile, pk)
# 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.
# ticket's own write rule and nothing wider. It used to add its own
# superuser clause; superusers now pass through `is_org_admin`, the
# same way on this path as on `CaseDetailView.patch`.
assert_case_write_access(request.profile, case)
serializer = CaseMoveSerializer(data=request.data)
@ -303,12 +308,16 @@ class CaseMoveView(APIView):
closed_on = case.closed_on
if new_status == "Closed" and case.status != "Closed":
closed_on = timezone.localdate()
refusal = duplicate_refusal(case.status, new_status) or close_refusal(
case,
status=new_status,
closed_on=closed_on,
priority=case.priority,
case_type=case.case_type,
refusal = (
merged_status_refusal(case, new_status)
or duplicate_refusal(case.status, new_status)
or close_refusal(
case,
status=new_status,
closed_on=closed_on,
priority=case.priority,
case_type=case.case_type,
)
)
if refusal:
return Response(
@ -383,7 +392,7 @@ class CasePipelineListCreateView(APIView):
"""Create a new pipeline."""
org = request.profile.org
if not is_org_admin(request.profile) and not request.user.is_superuser:
if not is_org_admin(request.profile):
return Response(
{"error": "Only admins can create pipelines"},
status=status.HTTP_403_FORBIDDEN,
@ -471,7 +480,7 @@ class CasePipelineDetailView(APIView):
responses={200: CasePipelineSerializer},
)
def put(self, request, pk):
if not is_org_admin(request.profile) and not request.user.is_superuser:
if not is_org_admin(request.profile):
return Response(
{"error": "Permission denied"}, status=status.HTTP_403_FORBIDDEN
)
@ -492,7 +501,7 @@ class CasePipelineDetailView(APIView):
@extend_schema(tags=["Case Pipelines"], responses={204: None})
def delete(self, request, pk):
if not is_org_admin(request.profile) and not request.user.is_superuser:
if not is_org_admin(request.profile):
return Response(
{"error": "Permission denied"}, status=status.HTTP_403_FORBIDDEN
)
@ -524,7 +533,7 @@ class CaseStageCreateView(APIView):
responses={201: CaseStageSerializer},
)
def post(self, request, pipeline_pk):
if not is_org_admin(request.profile) and not request.user.is_superuser:
if not is_org_admin(request.profile):
return Response(
{"error": "Permission denied"}, status=status.HTTP_403_FORBIDDEN
)
@ -557,7 +566,7 @@ class CaseStageDetailView(APIView):
responses={200: CaseStageSerializer},
)
def put(self, request, pk):
if not is_org_admin(request.profile) and not request.user.is_superuser:
if not is_org_admin(request.profile):
return Response(
{"error": "Permission denied"}, status=status.HTTP_403_FORBIDDEN
)
@ -576,7 +585,7 @@ class CaseStageDetailView(APIView):
@extend_schema(tags=["Case Stages"], responses={204: None})
def delete(self, request, pk):
if not is_org_admin(request.profile) and not request.user.is_superuser:
if not is_org_admin(request.profile):
return Response(
{"error": "Permission denied"}, status=status.HTTP_403_FORBIDDEN
)
@ -611,7 +620,7 @@ class CaseStageReorderView(APIView):
)
@transaction.atomic
def post(self, request, pipeline_pk):
if not is_org_admin(request.profile) and not request.user.is_superuser:
if not is_org_admin(request.profile):
return Response(
{"error": "Permission denied"}, status=status.HTTP_403_FORBIDDEN
)

View file

@ -17,6 +17,7 @@ from rest_framework.permissions import IsAuthenticated
from rest_framework.response import Response
from rest_framework.views import APIView
from cases.access import get_case_or_404
from cases.kb_text import snippet, text_match
from cases.models import Case, Solution
from common.permissions import HasOrgContext
@ -65,9 +66,10 @@ class SolutionSuggestionsView(APIView):
)
def get(self, request, pk):
org = request.profile.org
case = Case.objects.filter(pk=pk, org=org).first()
if case is None:
return Response({"error": "Case not found"}, status=404)
# Read access, not org alone: the suggestions are seeded from the
# ticket's own name and description, so a hidden ticket's subject
# would come back as the articles it matched.
case = get_case_or_404(request.profile, pk)
try:
limit = int(request.query_params.get("limit", _DEFAULT_LIMIT))

View file

@ -18,7 +18,7 @@ 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, get_case_or_404, visible_cases_qs
from cases.access import get_case_or_404, has_case_read_access, visible_cases_qs
from cases.models import Case, EmailMessage
from cases.notifications import case_link
from cases.serializer import CaseSerializer, parent_access_context
@ -52,8 +52,8 @@ class CaseMergeTargetsView(APIView):
Answers what the merge endpoint would accept and nothing wider: tickets the
caller can open, that `can_merge_case` allows (so a non-admin sees only
tickets they raised), that are live and not themselves merged, never the
ticket itself. The source takes the ticket's read rule first, so a ticket
the caller cannot open answers here exactly as its detail page does.
ticket itself. The source is looked up through the read rule, so a ticket
the caller cannot open answers here exactly as its detail page does: 404.
"""
permission_classes = (IsAuthenticated, HasOrgContext)
@ -73,7 +73,6 @@ class CaseMergeTargetsView(APIView):
def get(self, request, pk: str, format=None):
profile = request.profile
source = get_case_or_404(profile, pk)
assert_case_read_access(profile, source)
if not can_merge_case(profile, source):
return Response(
{
@ -159,7 +158,14 @@ class CaseMergeView(APIView):
source = by_id.get(str(pk))
target = by_id.get(str(into_id))
if not source or not target:
# A ticket the caller may not open, on either side, answers as a
# missing one does. The 403 below would confirm it exists.
if (
not source
or not target
or not has_case_read_access(request.profile, source)
or not has_case_read_access(request.profile, target)
):
return Response(
{"error": True, "errors": "Ticket not found."},
status=status.HTTP_404_NOT_FOUND,

View file

@ -24,7 +24,6 @@ Who may call them follows `cases.access`, the same as the ticket detail view:
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
from rest_framework import status
from rest_framework.permissions import IsAuthenticated
@ -32,14 +31,16 @@ from rest_framework.response import Response
from rest_framework.views import APIView
from cases.access import (
assert_case_read_access,
assert_case_write_access,
get_case_or_404,
has_case_read_access,
has_case_write_access,
lock_case_or_404,
)
from cases.approvals import close_refusal
from cases.models import Case
from cases.parent_guards import check_parent_link
from cases.workflow import MERGED_STATUS_LOCKED
from common.models import Activity
from common.permissions import HasOrgContext
@ -114,12 +115,9 @@ class CaseTreeView(APIView):
def get(self, request, pk):
org = request.profile.org
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)
# The same answer the ticket detail GET gives: 404 outside the org and
# for a ticket in it that the caller may not open.
case = get_case_or_404(request.profile, pk)
# 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
@ -144,9 +142,7 @@ class CaseLinkParentView(APIView):
parent_id = request.data.get("parent_id")
# Lock the case row so a parallel link from another agent cannot
# race on the cycle check. We look up the parent under the same lock.
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)
case = lock_case_or_404(request.profile, pk)
# 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)
@ -229,12 +225,18 @@ class CaseLinkParentView(APIView):
def _open_descendants(case, out=None, seen=None):
"""Collect all open (status != Closed) active descendants, depth-first.
"""Collect all open (neither Closed nor Duplicate) 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.
closed child is still collected. A merged child is passed over the same
way: its status is not the cascade's to change (`merged_status_refusal`),
and what it held now lives on the ticket it was merged into. The
``merged_into`` test is not redundant with Duplicate: a merged ticket
edited to another status before that rule existed is still merged. ``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 = []
@ -244,7 +246,11 @@ def _open_descendants(case, out=None, seen=None):
if child.id in seen:
continue
seen.add(child.id)
if child.status != "Closed" and child.is_active:
if (
child.status not in ("Closed", "Duplicate")
and child.is_active
and not child.merged_into_id
):
out.append(child)
_open_descendants(child, out, seen)
return out
@ -287,10 +293,10 @@ class CaseCloseWithChildrenView(APIView):
@transaction.atomic
def post(self, request, pk):
org = request.profile.org
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)
case = lock_case_or_404(request.profile, pk)
assert_case_write_access(request.profile, case)
if case.merged_into_id:
return _refused({"status": MERGED_STATUS_LOCKED})
if case.status == "Duplicate":
return Response(
{"detail": "Cannot close a merged case."},

View file

@ -17,7 +17,11 @@ from cases.models import (
TimeEntry,
)
from cases.parent_guards import check_parent_link
from cases.workflow import DUPLICATE_BY_MERGE_ONLY, duplicate_refusal
from cases.workflow import (
DUPLICATE_BY_MERGE_ONLY,
duplicate_refusal,
merged_status_refusal,
)
from common.models import Profile, Teams
from common.permissions import is_org_admin
from common.serializer import (
@ -267,6 +271,12 @@ class CaseCreateSerializer(serializers.ModelSerializer):
new_status = attrs.get("status", getattr(self.instance, "status", None))
# First, so a merged ticket hears "unmerge first" and not a close-gate
# message about a transition it may not make at all.
refusal = merged_status_refusal(self.instance, new_status)
if refusal:
raise serializers.ValidationError(refusal)
# Parent linking, only when this request carries `parent`. Judging the
# stored parent on every save would reject an ordinary rename of a case
# that sits in a tree, and would make a record uneditable if its stored

View file

@ -562,13 +562,17 @@ def _evaluate_reopen(case, comment):
Returns:
"reopened" if the case was reopened (and a REOPENED Activity emitted),
"out_of_window" if it would have reopened but closed_on is too old,
None otherwise (agent comment, internal note, non-Closed status, policy off).
None otherwise (agent comment, internal note, non-Closed status, merged
ticket, policy off).
"""
if getattr(comment, "is_internal", False):
return None
if comment.commented_by_id is not None:
return None
if case.status != REOPEN_TRIGGER_STATUS:
# A merged ticket's status is not changed by anything but an unmerge
# (`merged_status_refusal`). One can still be Closed if it was edited
# there before that rule existed.
if case.status != REOPEN_TRIGGER_STATUS or case.merged_into_id:
return None
if not case.closed_on:
return None
@ -628,7 +632,7 @@ def maybe_reopen_for_inbound_email(case, email_message):
Mirrors _evaluate_reopen but records the trigger as email_message_id.
Returns "reopened", "out_of_window", or None.
"""
if case.status != REOPEN_TRIGGER_STATUS:
if case.status != REOPEN_TRIGGER_STATUS or case.merged_into_id:
return None
if not case.closed_on:
return None

View file

@ -167,11 +167,12 @@ class TestInboxStillShowsTheFourWhoMaySee:
@pytest.mark.django_db
class TestRequestingApprovalNeedsCaseAccess:
def test_an_unrelated_member_is_refused(self, outsider_client, org_a, hidden_case):
"""404, as for a missing case: the member cannot open this one."""
_rule(org_a, approver_role="ADMIN")
response = outsider_client.post(
f"/api/cases/{hidden_case.id}/request-approval/", {}, format="json"
)
assert response.status_code == status.HTTP_403_FORBIDDEN
assert response.status_code == status.HTTP_404_NOT_FOUND
assert not Approval.objects.filter(case=hidden_case).exists()
def test_the_response_does_not_leak_the_case_name(

View file

@ -130,8 +130,14 @@ class TestWhoMayOpenACase:
assert response.status_code == status.HTTP_200_OK
def test_unrelated_member_of_the_org_is_refused(self, user_client, case_a):
"""404 with a missing id's body, not 403, which confirmed it exists."""
response = user_client.get(_detail(case_a.id))
assert response.status_code == status.HTTP_403_FORBIDDEN
missing = user_client.get(_detail("00000000-0000-0000-0000-000000000000"))
assert response.status_code == status.HTTP_404_NOT_FOUND
assert (response.status_code, response.json()) == (
missing.status_code,
missing.json(),
)
def test_another_org_gets_404_not_403(self, org_b_client, case_a):
"""404, not 403. Confirming a record exists is itself a disclosure."""
@ -254,9 +260,11 @@ class TestAttachmentDeleteIsScoped:
assert not Attachments.objects.filter(pk=mine.id).exists()
def test_someone_elses_upload_is_refused(
self, user_client, case_a, org_a, admin_user
self, user_client, watched_case, org_a, admin_user
):
theirs = self._attach(case_a, org_a, "theirs-in-org.txt")
"""403 for a reader of the ticket; one who cannot open it gets 404
(`test_case_comment_attachment_scope`)."""
theirs = self._attach(watched_case, org_a, "theirs-in-org.txt")
_created_by(theirs, admin_user)
response = user_client.delete(_attachment(theirs.id))
assert response.status_code == status.HTTP_403_FORBIDDEN

View file

@ -0,0 +1,172 @@
"""`/api/cases/comment/<pk>/` and `/api/cases/attachment/<pk>/` resolve
through a ticket the caller may open.
Both looked the row up across the whole org, in the generic tables every module
shares, and then answered 403 to a non-author. So the 403 confirmed that a
comment or attachment existed on a ticket (or a lead, or a deal) the caller
cannot open, and an author who had lost the ticket could still edit or delete
there. Now a row on a hidden ticket, or on another module's record, answers
exactly like an id that does not exist (compared whole: status and body), and
the author-or-admin 403 is reserved for readers of the ticket.
"""
import pytest
from crum import impersonate
from django.contrib.contenttypes.models import ContentType
from django.core.files.uploadedfile import SimpleUploadedFile
from cases.models import Case, CaseWatcher
from common.models import Attachments, Comment
from leads.models import Lead
MISSING = "00000000-0000-0000-0000-00000000c0de"
COMMENT = "/api/cases/comment/{}/"
ATTACHMENT = "/api/cases/attachment/{}/"
@pytest.fixture
def ticket(admin_user, org_a):
return Case.objects.create(
name="Payroll dispute",
status="New",
priority="Normal",
org=org_a,
created_by=admin_user,
)
@pytest.fixture
def lead(org_a):
return Lead.objects.create(title="A lead", first_name="Ay", org=org_a)
def _comment(record, author_profile, org):
return Comment.objects.create(
comment="Secret note",
content_type=ContentType.objects.get_for_model(record),
object_id=record.id,
commented_by=author_profile,
org=org,
)
def _attachment(record, creator, org):
# `BaseModel.save()` stamps `created_by` from crum's current user.
with impersonate(creator):
return Attachments.objects.create(
file_name="payslip.pdf",
attachment=SimpleUploadedFile("payslip.pdf", b"pdf"),
content_type=ContentType.objects.get_for_model(record),
object_id=record.id,
org=org,
)
def _whole(response):
return response.status_code, response.content
def _comment_calls(client, pk):
return [
client.put(COMMENT.format(pk), {"comment": "edited"}, format="json"),
client.patch(COMMENT.format(pk), {"comment": "edited"}, format="json"),
client.delete(COMMENT.format(pk)),
]
class TestHiddenAnswersLikeMissing:
def test_a_comment_on_a_ticket_the_caller_cannot_open(
self, user_client, user_profile, ticket, org_a
):
# Even their own comment, left before they lost the ticket.
note = _comment(ticket, user_profile, org_a)
for got, missing in zip(
_comment_calls(user_client, note.id), _comment_calls(user_client, MISSING)
):
assert got.status_code == 404
assert _whole(got) == _whole(missing)
note.refresh_from_db()
assert note.comment == "Secret note"
def test_a_comment_on_another_modules_record(
self, admin_client, admin_profile, lead, org_a
):
"""Even an admin: this route is for ticket comments only."""
note = _comment(lead, admin_profile, org_a)
for got, missing in zip(
_comment_calls(admin_client, note.id),
_comment_calls(admin_client, MISSING),
):
assert got.status_code == 404
assert _whole(got) == _whole(missing)
assert Comment.objects.filter(id=note.id).exists()
def test_an_attachment_on_a_ticket_the_caller_cannot_open(
self, user_client, regular_user, ticket, org_a
):
att = _attachment(ticket, regular_user, org_a)
got = user_client.delete(ATTACHMENT.format(att.id))
missing = user_client.delete(ATTACHMENT.format(MISSING))
assert got.status_code == 404
assert _whole(got) == _whole(missing)
assert Attachments.objects.filter(id=att.id).exists()
def test_an_attachment_on_another_modules_record(
self, admin_client, admin_user, lead, org_a
):
att = _attachment(lead, admin_user, org_a)
got = admin_client.delete(ATTACHMENT.format(att.id))
missing = admin_client.delete(ATTACHMENT.format(MISSING))
assert got.status_code == 404
assert _whole(got) == _whole(missing)
assert Attachments.objects.filter(id=att.id).exists()
class TestReadersOfTheTicket:
@pytest.fixture
def reader(self, user_profile, ticket, org_a):
CaseWatcher.objects.create(case=ticket, profile=user_profile, org=org_a)
return user_profile
def test_a_reader_may_not_edit_someone_elses_comment(
self, user_client, reader, admin_profile, ticket, org_a
):
note = _comment(ticket, admin_profile, org_a)
for response in _comment_calls(user_client, note.id):
assert response.status_code == 403
assert Comment.objects.get(id=note.id).comment == "Secret note"
def test_a_reader_edits_and_deletes_their_own_comment(
self, user_client, reader, ticket, org_a
):
note = _comment(ticket, reader, org_a)
response = user_client.patch(
COMMENT.format(note.id), {"comment": "edited"}, format="json"
)
assert response.status_code == 200, response.content
assert Comment.objects.get(id=note.id).comment == "edited"
assert user_client.delete(COMMENT.format(note.id)).status_code == 200
assert not Comment.objects.filter(id=note.id).exists()
def test_an_admin_edits_anyones_comment_on_a_ticket(
self, admin_client, user_profile, ticket, org_a
):
note = _comment(ticket, user_profile, org_a)
response = admin_client.put(
COMMENT.format(note.id), {"comment": "tidied"}, format="json"
)
assert response.status_code == 200, response.content
def test_a_reader_may_not_delete_someone_elses_attachment(
self, user_client, reader, admin_user, ticket, org_a
):
att = _attachment(ticket, admin_user, org_a)
assert user_client.delete(ATTACHMENT.format(att.id)).status_code == 403
assert Attachments.objects.filter(id=att.id).exists()
def test_a_reader_deletes_their_own_attachment(
self, user_client, reader, regular_user, ticket, org_a
):
att = _attachment(ticket, regular_user, org_a)
assert user_client.delete(ATTACHMENT.format(att.id)).status_code == 200
assert not Attachments.objects.filter(id=att.id).exists()

View file

@ -60,9 +60,9 @@ EXPECTED = {
"creator": {"Created"},
# A watcher may open the ticket, so it is theirs to export too.
"unrelated": {"Watched"},
# `visible_cases_qs` reads the role only; a superuser holding a member
# profile gets a member's queue, in the list and in the file alike.
"superuser": set(),
# A superuser is an org admin (`is_org_admin`) even on a member profile,
# so they get the whole queue, in the list and in the file alike.
"superuser": EVERYONE,
}

View file

@ -207,10 +207,13 @@ class TestCaseMergePermissions:
assert response.status_code == 200
def test_creator_of_only_one_forbidden(
self, user_client, regular_user, admin_user, org_a
self, user_client, regular_user, user_profile, admin_user, org_a
):
a = _make_case(org_a, regular_user, name="A. User-created")
b = _make_case(org_a, admin_user, name="B, admin-created")
# Readable, so the refusal is the merge rule's 403 and not the 404 a
# ticket the caller cannot open gets.
b.assigned_to.add(user_profile)
response = user_client.post(f"/api/cases/{a.id}/merge/{b.id}/")
assert response.status_code == 403

View file

@ -0,0 +1,128 @@
"""`visible_cases_qs` admits superusers, and the analytics read through it.
Two follow-ups from the 1.10.0 pass, pinned both ways:
* `visible_cases_qs` ignored ``is_superuser`` while its siblings admitted it.
It goes through `is_org_admin`, which now admits a superuser's profile, so a
superuser on a USER profile sees every ticket in the org and a plain member
still sees only their own.
* `cases/analytics_views.py` carried an inline copy of the read rule (D40). It
now uses `visible_cases_qs`, so a member's figures count only tickets they
can open, and an admin's or a superuser's count them all.
"""
from datetime import timedelta
import pytest
from django.utils import timezone
from cases.access import visible_cases_qs
from cases.models import Case, CaseWatcher
from conftest import rls_org
@pytest.fixture
def superuser(regular_user):
"""The `user_client` caller, still on a USER profile, made superuser."""
regular_user.is_superuser = True
regular_user.save(update_fields=["is_superuser"])
return regular_user
@pytest.fixture
def tickets(admin_user, regular_user, user_profile, org_a):
"""Four tickets answered yesterday: the member created one, is assigned
one, watches one, and has nothing to do with the fourth."""
yesterday = timezone.now() - timedelta(days=1)
def make(name, creator):
case = Case.objects.create(
name=name,
status="New",
priority="Normal",
org=org_a,
created_by=creator,
sla_first_response_hours=4,
)
Case.objects.filter(pk=case.pk).update(
created_at=yesterday, first_response_at=yesterday + timedelta(hours=1)
)
return case
created = make("Created", regular_user)
assigned = make("Assigned", admin_user)
assigned.assigned_to.add(user_profile)
watched = make("Watched", admin_user)
CaseWatcher.objects.create(case=watched, profile=user_profile, org=org_a)
hidden = make("Hidden", admin_user)
return {
"created": created,
"assigned": assigned,
"watched": watched,
"hidden": hidden,
}
def _names(qs):
return set(qs.values_list("name", flat=True))
ALL = {"Created", "Assigned", "Watched", "Hidden"}
MEMBERS = {"Created", "Assigned", "Watched"}
class TestVisibleCasesQs:
def test_a_plain_member_sees_only_their_own(self, user_profile, tickets):
assert _names(visible_cases_qs(user_profile)) == MEMBERS
def test_a_superuser_on_a_member_profile_sees_every_ticket(
self, superuser, user_profile, tickets
):
user_profile.refresh_from_db()
assert _names(visible_cases_qs(user_profile)) == ALL
def test_the_detail_view_agrees(self, user_client, tickets, request):
hidden = tickets["hidden"]
assert user_client.get(f"/api/cases/{hidden.id}/").status_code == 404
request.getfixturevalue("superuser")
assert user_client.get(f"/api/cases/{hidden.id}/").status_code == 200
def test_another_orgs_ticket_stays_out_for_a_superuser(
self, superuser, user_profile, user_b, org_b, tickets
):
with rls_org(org_b):
Case.objects.create(
name="Foreign", status="New", priority="Normal", org=org_b
)
assert "Foreign" not in _names(visible_cases_qs(user_profile))
FRT = "/api/cases/analytics/frt/"
DRILL = "/api/cases/analytics/drilldown/"
class TestAnalyticsFollowTheReadRule:
def test_a_members_figures_leave_out_what_they_cannot_open(
self, user_client, tickets
):
assert user_client.get(FRT).data["count"] == 3
ids = {
row["id"]
for row in user_client.get(DRILL, {"metric": "frt"}).data["results"]
}
assert ids == {str(tickets[k].id) for k in ("created", "assigned", "watched")}
def test_an_admin_counts_everything(self, admin_client, tickets):
assert admin_client.get(FRT).data["count"] == 4
def test_a_superuser_counts_everything(self, superuser, user_client, tickets):
assert user_client.get(FRT).data["count"] == 4
drilled = user_client.get(DRILL, {"metric": "frt"}).data["results"]
assert {row["name"] for row in drilled} == ALL
def test_the_service_overview_admits_a_superuser_and_refuses_a_member(
self, user_client, tickets, request
):
assert user_client.get("/api/cases/analytics/service/").status_code == 403
request.getfixturevalue("superuser")
assert user_client.get("/api/cases/analytics/service/").status_code == 200

View file

@ -16,7 +16,7 @@ from django.utils import timezone
from rest_framework import status
from accounts.models import Account
from cases.models import Case, CasePipeline, CaseStage, Solution
from cases.models import Case, CasePipeline, CaseStage, CaseWatcher, Solution
from common.models import Attachments, Comment, Tags, Teams
from contacts.models import Contact
@ -514,31 +514,26 @@ class TestCaseDetailView:
)
assert response.status_code == status.HTTP_400_BAD_REQUEST
def test_update_case_non_admin_forbidden(self, user_client, case_a):
"""Non-admin who is not creator/assignee should get 403."""
def test_update_case_non_admin_who_cannot_open_it_is_404(self, user_client, case_a):
"""Non-admin who is not creator/assignee/watcher cannot open it: 404."""
response = user_client.put(
_detail_url(case_a.pk),
{"name": "Hacked", "status": "New", "priority": "Normal"},
format="json",
)
assert response.status_code == status.HTTP_403_FORBIDDEN
assert response.status_code == status.HTTP_404_NOT_FOUND
def test_delete_case_non_admin_not_creator(self, user_client, case_a):
"""Non-admin who is not creator should get 403 on delete."""
"""Non-admin who cannot open the case gets 404 on delete."""
response = user_client.delete(_detail_url(case_a.pk))
assert response.status_code == status.HTTP_403_FORBIDDEN
assert response.status_code == status.HTTP_404_NOT_FOUND
def test_get_detail_non_admin_forbidden(self, user_client, case_a):
"""Non-admin who is not creator/assignee/watcher should get 403.
The status is unchanged; the body is now DRF's standard
`{"detail": ...}` because the view raises `PermissionDenied` from the
one rule in `cases.access` instead of hand-building an envelope in
five places. Same shape as leads, opportunity, accounts and contacts.
"""
def test_get_detail_non_admin_who_cannot_open_it_is_404(self, user_client, case_a):
"""Non-admin who is not creator/assignee/watcher gets a missing
ticket's 404. A 403 here confirmed the ticket existed."""
response = user_client.get(_detail_url(case_a.pk))
assert response.status_code == status.HTTP_403_FORBIDDEN
assert "Permission" in response.data["detail"]
assert response.status_code == status.HTTP_404_NOT_FOUND
assert response.data["detail"] == "No such case."
def test_get_detail_non_admin_as_assignee(
self, user_client, admin_user, user_profile, org_a
@ -618,23 +613,25 @@ class TestCaseDetailView:
assert case_a.assigned_to.count() == 0
assert case_a.tags.count() == 0
def test_patch_case_non_admin_forbidden(self, user_client, case_a):
"""Non-admin who is not creator/assignee should get 403 on PATCH."""
def test_patch_case_non_admin_who_cannot_open_it_is_404(self, user_client, case_a):
"""Non-admin who cannot open the case gets 404 on PATCH."""
response = user_client.patch(
_detail_url(case_a.pk),
{"status": "Assigned"},
format="json",
)
assert response.status_code == status.HTTP_403_FORBIDDEN
assert response.status_code == status.HTTP_404_NOT_FOUND
def test_post_comment_non_admin_forbidden(self, user_client, case_a):
"""Non-admin who is not creator/assignee should get 403 on POST (comment)."""
def test_post_comment_non_admin_who_cannot_open_it_is_404(
self, user_client, case_a
):
"""Non-admin who cannot open the case gets 404 on POST (comment)."""
response = user_client.post(
_detail_url(case_a.pk),
{"comment": "Should not work"},
format="json",
)
assert response.status_code == status.HTTP_403_FORBIDDEN
assert response.status_code == status.HTTP_404_NOT_FOUND
def test_post_comment_non_admin_as_assignee(
self, user_client, admin_user, user_profile, org_a
@ -728,12 +725,15 @@ class TestCaseCommentView:
assert not Comment.objects.filter(id=comment.id).exists()
def test_update_comment_non_admin_forbidden(
self, user_client, admin_user, admin_profile, org_a
self, user_client, user_profile, admin_user, admin_profile, org_a
):
"""Non-admin who did not create comment should get 403."""
_case, comment = self._create_case_with_comment(
admin_user, admin_profile, org_a
)
# A reader of the ticket (a watcher): someone who cannot open it
# gets 404 (`test_case_comment_attachment_scope`).
CaseWatcher.objects.create(case=_case, profile=user_profile, org=org_a)
response = user_client.put(
f"/api/cases/comment/{comment.id}/",
{"comment": "Should fail"},
@ -742,12 +742,15 @@ class TestCaseCommentView:
assert response.status_code == status.HTTP_403_FORBIDDEN
def test_delete_comment_non_admin_forbidden(
self, user_client, admin_user, admin_profile, org_a
self, user_client, user_profile, admin_user, admin_profile, org_a
):
"""Non-admin who did not create the comment should get 403."""
_case, comment = self._create_case_with_comment(
admin_user, admin_profile, org_a
)
# A reader of the ticket (a watcher): someone who cannot open it
# gets 404 (`test_case_comment_attachment_scope`).
CaseWatcher.objects.create(case=_case, profile=user_profile, org=org_a)
response = user_client.delete(f"/api/cases/comment/{comment.id}/")
assert response.status_code == status.HTTP_403_FORBIDDEN
@ -790,7 +793,7 @@ class TestCaseCommentView:
assert comment.comment == "Original comment"
def test_a_stranger_still_gets_403_for_an_empty_body(
self, user_client, admin_user, admin_profile, org_a
self, user_client, user_profile, admin_user, admin_profile, org_a
):
"""Authorization is still checked first, and still wins.
@ -801,6 +804,9 @@ class TestCaseCommentView:
_case, comment = self._create_case_with_comment(
admin_user, admin_profile, org_a
)
# A reader of the ticket (a watcher): someone who cannot open it
# gets 404 (`test_case_comment_attachment_scope`).
CaseWatcher.objects.create(case=_case, profile=user_profile, org=org_a)
response = user_client.put(
f"/api/cases/comment/{comment.id}/",
{},
@ -839,7 +845,7 @@ class TestCaseAttachmentView:
assert not Attachments.objects.filter(id=attachment.id).exists()
def test_delete_attachment_non_admin_forbidden(
self, user_client, admin_user, org_a
self, user_client, user_profile, admin_user, org_a
):
"""Non-admin who did not create attachment should get 403."""
from django.core.files.uploadedfile import SimpleUploadedFile
@ -860,6 +866,8 @@ class TestCaseAttachmentView:
created_by=admin_user,
org=org_a,
)
# A reader of the ticket; one who cannot open it gets 404.
CaseWatcher.objects.create(case=case, profile=user_profile, org=org_a)
response = user_client.delete(f"/api/cases/attachment/{attachment.id}/")
assert response.status_code == status.HTTP_403_FORBIDDEN
@ -1361,12 +1369,15 @@ class TestCaseCommentPatchCoverage:
return case, comment
def test_patch_comment_non_admin_forbidden(
self, user_client, admin_user, admin_profile, org_a
self, user_client, user_profile, admin_user, admin_profile, org_a
):
"""Non-admin who didn't create comment gets 403 on PATCH (line 824-830)."""
_case, comment = self._create_case_with_comment(
admin_user, admin_profile, org_a
)
# A reader of the ticket (a watcher): someone who cannot open it
# gets 404 (`test_case_comment_attachment_scope`).
CaseWatcher.objects.create(case=_case, profile=user_profile, org=org_a)
response = user_client.patch(
f"/api/cases/comment/{comment.id}/",
{"comment": "Patched by non-admin"},

View file

@ -3,7 +3,7 @@
import pytest
from cases.approvals import ApprovalRule
from cases.models import Case
from cases.models import Case, CaseWatcher
from conftest import rls_org
@ -219,9 +219,13 @@ class TestBulkUpdateCasesCloseGate:
@pytest.mark.django_db
class TestBulkUpdatePerRecord:
def test_mixed_access_reports_both(self, user_client, regular_user, org_a, case_a):
def test_mixed_access_reports_both(
self, user_client, regular_user, user_profile, org_a, case_a
):
# `case_a` was created by admin_user, so the regular member cannot write
# it. A case they own can be written.
# it; they watch it, so they may open it and hear `no_access`. A case
# they own can be written.
CaseWatcher.objects.create(case=case_a, profile=user_profile, org=org_a)
mine = Case.objects.create(
name="Mine",
status="New",
@ -384,8 +388,10 @@ class TestBulkDeleteCasesAuthz:
@pytest.mark.django_db
class TestBulkDeletePerRecord:
def test_reports_deleted_and_no_access(
self, user_client, regular_user, org_a, case_a
self, user_client, regular_user, user_profile, org_a, case_a
):
# Watched, so readable: `no_access` is for a ticket the caller may open.
CaseWatcher.objects.create(case=case_a, profile=user_profile, org=org_a)
mine = Case.objects.create(
name="Mine",
status="New",

View file

@ -567,7 +567,7 @@ class TestCaseMove:
def test_move_case_non_admin_permission_denied(
self, user_client, admin_user, org_a
):
"""Non-admin who is not creator/assignee should get 403."""
"""Non-admin who cannot even open the case gets its detail page's 404."""
case = Case.objects.create(
name="Move Deny Case",
status="New",
@ -580,7 +580,7 @@ class TestCaseMove:
{"status": "Assigned"},
format="json",
)
assert response.status_code == status.HTTP_403_FORBIDDEN
assert response.status_code == status.HTTP_404_NOT_FOUND
def test_move_case_non_admin_as_assignee_allowed(
self, user_client, admin_user, user_profile, org_a

View file

@ -264,26 +264,51 @@ class TestThrottle:
assert anon.get(list_url()).status_code == 200
assert anon.get(list_url()).status_code == 429
def test_buckets_are_per_forwarded_client(self, anon, acme, monkeypatch):
def test_buckets_are_per_client(self, anon, acme, monkeypatch):
monkeypatch.setattr(
HelpCenterIPThrottle, "THROTTLE_RATES", {"help_center_ip": "1/hour"}
)
first = anon.get(list_url(), REMOTE_ADDR="198.51.100.1")
second = anon.get(list_url(), REMOTE_ADDR="198.51.100.2")
assert (first.status_code, second.status_code) == (200, 200)
def test_a_forged_forwarded_address_is_not_a_new_bucket(
self, anon, acme, monkeypatch
):
monkeypatch.setattr(
HelpCenterIPThrottle, "THROTTLE_RATES", {"help_center_ip": "1/hour"}
)
first = anon.get(list_url(), HTTP_X_FORWARDED_FOR="203.0.113.1")
second = anon.get(list_url(), HTTP_X_FORWARDED_FOR="203.0.113.2")
assert (first.status_code, second.status_code) == (200, 200)
assert (first.status_code, second.status_code) == (200, 429)
def test_rotating_the_forwarded_address_hits_the_global_limit(
self, anon, acme, monkeypatch
def test_behind_one_proxy_its_entry_is_the_bucket(
self, anon, acme, monkeypatch, settings
):
"""X-Forwarded-For is the caller's to set, so each request below gets a
fresh per-visitor bucket; the per-help-center one still fills."""
"""With NUM_PROXIES=1 the entry the proxy appended decides the bucket;
what the caller put to its left does not."""
settings.REST_FRAMEWORK = {**settings.REST_FRAMEWORK, "NUM_PROXIES": 1}
monkeypatch.setattr(
HelpCenterIPThrottle, "THROTTLE_RATES", {"help_center_ip": "1/hour"}
)
proxy = {"REMOTE_ADDR": "127.0.0.1"}
a = anon.get(list_url(), HTTP_X_FORWARDED_FOR="6.6.6.6, 198.51.100.1", **proxy)
b = anon.get(list_url(), HTTP_X_FORWARDED_FOR="198.51.100.2", **proxy)
a_forged = anon.get(
list_url(), HTTP_X_FORWARDED_FOR="7.7.7.7, 198.51.100.1", **proxy
)
assert [a.status_code, b.status_code, a_forged.status_code] == [200, 200, 429]
def test_many_real_clients_hit_the_global_limit(self, anon, acme, monkeypatch):
"""Each address below gets its own per-visitor bucket; the
per-help-center one still fills."""
monkeypatch.setattr(
HelpCenterGlobalThrottle,
"THROTTLE_RATES",
{"help_center_global": "2/hour"},
)
codes = [
anon.get(list_url(), HTTP_X_FORWARDED_FOR=f"203.0.113.{n}").status_code
anon.get(list_url(), REMOTE_ADDR=f"198.51.100.{n}").status_code
for n in range(1, 4)
]
assert codes == [200, 200, 429]

View file

@ -0,0 +1,169 @@
"""A same-org ticket the caller cannot open answers like a missing one.
Owner decision (1.11.0): on every endpoint that takes a ticket id, a ticket in
the caller's org that they may not read answers 404 with a body identical to
the one a missing id gets. A 403 confirmed the ticket existed. The responses
are compared whole, status and body, never by status alone, and the hidden
ticket is checked untouched afterwards.
The caller is a plain member who did not create the hidden ticket, is not
assigned to it and does not watch it. `test_case_access_and_sla` pins the
other direction: a watcher, who may read, still gets 403 on a write.
"""
import pytest
from cases.models import Case, CaseWatcher, TimeEntry
from common.models import Comment
MISSING = "00000000-0000-0000-0000-00000000abcd"
OTHER = "00000000-0000-0000-0000-00000000ef01"
# (method, path under /api/cases/<id>/, body)
ENDPOINTS = [
("get", "", None),
("put", "", {"name": "Renamed", "status": "New", "priority": "Normal"}),
("patch", "", {"priority": "Low"}),
("post", "", {"comment": "hello"}),
("delete", "", None),
("get", "activities/", None),
("post", "solutions/", {"solution_id": OTHER}),
("delete", f"solutions/{OTHER}/", None),
("get", "solution-suggestions/", None),
("post", "watch/", None),
("delete", "watch/", None),
("get", "watchers/", None),
("get", "merge-targets/", None),
("post", "unmerge/", None),
("get", "tree/", None),
("post", "link/", {"parent_id": None}),
("post", "close-with-children/", {"cascade": False}),
("get", "time-entries/", None),
("post", "time-entries/", {"duration_minutes": 30, "description": "x"}),
("post", "time-entries/start/", None),
("get", "time-summary/", None),
("post", "request-approval/", {}),
("patch", "move/", {"status": "Assigned"}),
]
@pytest.fixture
def hidden(admin_user, org_a):
return Case.objects.create(
name="Payroll dispute",
status="New",
priority="Normal",
org=org_a,
created_by=admin_user,
)
@pytest.fixture
def mine(regular_user, org_a):
return Case.objects.create(
name="My ticket",
status="New",
priority="Normal",
org=org_a,
created_by=regular_user,
)
def _call(client, method, url, body):
return getattr(client, method)(url, body, format="json")
def _whole(response):
return response.status_code, response.content
def _untouched(case):
case.refresh_from_db()
assert case.status == "New"
assert case.name == "Payroll dispute"
assert case.priority == "Normal"
assert case.is_active is True
assert case.parent_id is None
assert not CaseWatcher.objects.filter(case=case).exists()
assert not TimeEntry.objects.filter(case=case).exists()
assert not Comment.objects.filter(object_id=case.id).exists()
@pytest.mark.parametrize(("method", "path", "body"), ENDPOINTS)
def test_hidden_answers_exactly_like_missing(method, path, body, user_client, hidden):
got = _call(user_client, method, f"/api/cases/{hidden.id}/{path}", body)
missing = _call(user_client, method, f"/api/cases/{MISSING}/{path}", body)
assert got.status_code == 404, got.content
assert _whole(got) == _whole(missing)
assert b"Payroll dispute" not in got.content
_untouched(hidden)
@pytest.mark.parametrize(("method", "path", "body"), ENDPOINTS)
def test_a_reader_is_not_told_404(
method, path, body, user_client, user_profile, hidden, org_a
):
"""The other side of the rule: once the member may read the ticket (here,
as its assignee) no endpoint above answers what a missing ticket gets.
(Two of them still 404, about the solution id in the path or body.)"""
hidden.assigned_to.add(user_profile)
got = _call(user_client, method, f"/api/cases/{hidden.id}/{path}", body)
missing = _call(user_client, method, f"/api/cases/{MISSING}/{path}", body)
assert _whole(got) != _whole(missing), (path, got.content)
def test_merging_a_hidden_source_answers_like_a_missing_one(user_client, hidden, mine):
got = user_client.post(f"/api/cases/{hidden.id}/merge/{mine.id}/")
missing = user_client.post(f"/api/cases/{MISSING}/merge/{mine.id}/")
assert got.status_code == 404
assert _whole(got) == _whole(missing)
_untouched(hidden)
def test_merging_into_a_hidden_target_answers_like_a_missing_one(
user_client, hidden, mine
):
got = user_client.post(f"/api/cases/{mine.id}/merge/{hidden.id}/")
missing = user_client.post(f"/api/cases/{mine.id}/merge/{MISSING}/")
assert got.status_code == 404
assert _whole(got) == _whole(missing)
mine.refresh_from_db()
assert mine.merged_into_id is None
class TestBulk:
"""A batch leaves a hidden id out of `results`, exactly as a missing one."""
def test_update_reports_nothing_for_a_hidden_id(self, user_client, hidden, mine):
response = user_client.post(
"/api/cases/bulk/update/",
{
"ids": [str(hidden.id), MISSING, str(mine.id)],
"fields": {"priority": "Low"},
},
format="json",
)
assert response.json()["results"] == [{"id": str(mine.id), "status": "updated"}]
_untouched(hidden)
def test_delete_reports_nothing_for_a_hidden_id(self, user_client, hidden, mine):
response = user_client.post(
"/api/cases/bulk/delete/",
{"ids": [str(hidden.id), MISSING, str(mine.id)]},
format="json",
)
assert response.json()["results"] == [{"id": str(mine.id), "status": "deleted"}]
_untouched(hidden)
def test_a_readable_but_unwritable_ticket_still_reports_no_access(
self, user_client, user_profile, hidden, org_a
):
CaseWatcher.objects.create(case=hidden, profile=user_profile, org=org_a)
response = user_client.post(
"/api/cases/bulk/update/",
{"ids": [str(hidden.id)], "fields": {"priority": "Low"}},
format="json",
)
assert response.json()["results"] == [
{"id": str(hidden.id), "status": "no_access"}
]

View file

@ -2,10 +2,10 @@
`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`.
assignee, a superuser clause of its own that `CaseDetailView.patch` did not
have. The move now asks `assert_case_write_access`, and a superuser is an org
admin through `is_org_admin` on both paths alike (owner decision, 1.11.0). A
ticket the caller may not even open answers 404, as its detail page does.
"""
import pytest
@ -46,18 +46,21 @@ def _assert_unchanged(case):
class TestSuperuserPlainMember:
def test_refused_on_a_ticket_they_cannot_edit(
def test_admin_on_the_board_as_on_the_detail(
self, superuser, user_client, others_case
):
"""A superuser on a USER profile may move a ticket they neither
created nor hold, and the detail PATCH agrees."""
response = _move(user_client, others_case)
assert response.status_code == 200, response.content
others_case.refresh_from_db()
assert others_case.status == "Assigned"
assert (
user_client.patch(
f"/api/cases/{others_case.id}/", {"status": "Assigned"}, format="json"
f"/api/cases/{others_case.id}/", {"status": "New"}, format="json"
).status_code
== 403
== 200
)
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
@ -107,9 +110,16 @@ class TestOrdinaryRoles:
others_case.refresh_from_db()
assert others_case.status == "Assigned"
def test_member_without_write_access_is_refused(self, user_client, others_case):
def test_member_who_cannot_open_it_gets_a_missing_tickets_answer(
self, user_client, others_case
):
response = _move(user_client, others_case)
assert response.status_code == 403, response.content
missing = _move(user_client, Case(id="00000000-0000-0000-0000-000000000000"))
assert response.status_code == 404, response.content
assert (response.status_code, response.json()) == (
missing.status_code,
missing.json(),
)
_assert_unchanged(others_case)
def test_watcher_reads_but_may_not_move(

View file

@ -157,7 +157,7 @@ class TestMergeTargets:
):
hidden = _case(org_a, admin_user, "Hidden")
detail = user_client.get(f"/api/cases/{hidden.id}/").status_code
assert _targets(user_client, hidden).status_code == detail == 403
assert _targets(user_client, hidden).status_code == detail == 404
def test_another_orgs_source_is_404(self, admin_client, user_b, org_b):
with rls_org(org_b):
@ -175,9 +175,9 @@ class TestMergeTargets:
def test_board_move_of_an_unwritable_ticket_answers_like_its_detail(
who, user_client, user_profile, admin_user, org_a
):
"""A hidden ticket's move answers what its detail GET answers (403 inside
the org, `test_kanban_move_write_rule` pins 404 across orgs), and a
watcher, who may read it, is still refused the move. Nothing is written."""
"""A hidden ticket's move answers what its detail GET answers (404, inside
the org as across it), and a watcher, who may read it, is refused the move
with a 403. Nothing is written."""
case = _case(org_a, admin_user, "Not mine")
if who == "watcher":
CaseWatcher.objects.create(case=case, profile=user_profile, org=org_a)
@ -185,7 +185,7 @@ def test_board_move_of_an_unwritable_ticket_answers_like_its_detail(
move = user_client.patch(
f"/api/cases/{case.id}/move/", {"status": "Closed"}, format="json"
)
assert move.status_code == 403
assert detail == (200 if who == "watcher" else 403)
assert move.status_code == (403 if who == "watcher" else 404)
assert detail == (200 if who == "watcher" else 404)
case.refresh_from_db()
assert case.status == "New"

View file

@ -53,7 +53,7 @@ class TestMergedFrom:
# The source really is hidden from this member.
assert (
user_client.get(f"/api/cases/{hidden.id}/?show_merged=true").status_code
== 403
== 404
)
resp = user_client.get(f"/api/cases/{target.id}/")

View file

@ -34,7 +34,9 @@ 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
missing = user_client.get("/api/cases/00000000-0000-0000-0000-000000000000/")
assert response.status_code == 404
assert response.json() == missing.json()
body = response.content.decode()
assert "Secret duplicate" not in body
assert str(target.id) not in body

View file

@ -0,0 +1,238 @@
"""A merged ticket's status changes only by unmerging it (owner decision, 1.11.0).
Editing a merged ticket (``merged_into`` set) to another status used to keep
``merged_into``, so the ticket was open and merged at once. Every path that
writes a ticket's status now refuses that with the same sentence, and each
refusal here is paired with the same request on an unmerged ticket going
through.
"""
from datetime import timedelta
import pytest
from django.contrib.contenttypes.models import ContentType
from django.utils import timezone
from cases.models import Case
from cases.workflow import MERGED_STATUS_LOCKED
from common.models import Comment
from conftest import rls_org
def _case(org, creator, name, **extra):
fields = {"status": "New", "priority": "Normal"}
fields.update(extra)
return Case.objects.create(name=name, org=org, created_by=creator, **fields)
@pytest.fixture
def merged(admin_client, admin_user, org_a):
"""A ticket merged into another through the real endpoint."""
target = _case(org_a, admin_user, "Survivor")
source = _case(org_a, admin_user, "Duplicate of survivor")
response = admin_client.post(f"/api/cases/{source.id}/merge/{target.id}/")
assert response.status_code == 200, response.content
source.refresh_from_db()
assert source.merged_into_id == target.id and source.status == "Duplicate"
return source
@pytest.fixture
def plain(admin_user, org_a):
return _case(org_a, admin_user, "Ordinary ticket")
def _detail(case):
return f"/api/cases/{case.id}/"
def _unchanged(case):
before = case.status
case.refresh_from_db()
assert case.status == before
assert case.merged_into_id is not None
class TestDetailEdit:
def test_patch_status_on_a_merged_ticket_is_refused(self, admin_client, merged):
response = admin_client.patch(
_detail(merged), {"status": "Assigned"}, format="json"
)
assert response.status_code == 400
assert response.json()["errors"] == {"status": [MERGED_STATUS_LOCKED]}
_unchanged(merged)
def test_put_status_on_a_merged_ticket_is_refused(self, admin_client, merged):
response = admin_client.put(
_detail(merged),
{"name": merged.name, "status": "Closed", "priority": "Normal"},
format="json",
)
assert response.status_code == 400
assert MERGED_STATUS_LOCKED in str(response.json())
_unchanged(merged)
def test_other_fields_on_a_merged_ticket_still_save(self, admin_client, merged):
"""Re-sending the status it has is not a change of status."""
response = admin_client.put(
_detail(merged),
{
"name": merged.name,
"status": "Duplicate",
"priority": "High",
"description": "Kept for the record.",
},
format="json",
)
assert response.status_code == 200, response.content
merged.refresh_from_db()
assert merged.priority == "High"
assert merged.status == "Duplicate"
def test_an_unmerged_ticket_changes_status(self, admin_client, plain):
response = admin_client.patch(
_detail(plain), {"status": "Assigned"}, format="json"
)
assert response.status_code == 200, response.content
plain.refresh_from_db()
assert plain.status == "Assigned"
def test_after_unmerge_the_status_changes_again(self, admin_client, merged):
response = admin_client.post(f"/api/cases/{merged.id}/unmerge/")
assert response.status_code == 200, response.content
response = admin_client.patch(
_detail(merged), {"status": "Assigned"}, format="json"
)
assert response.status_code == 200, response.content
merged.refresh_from_db()
assert merged.status == "Assigned" and merged.merged_into_id is None
class TestBoardMove:
def test_move_of_a_merged_ticket_is_refused(self, admin_client, merged):
response = admin_client.patch(
f"/api/cases/{merged.id}/move/", {"status": "Assigned"}, format="json"
)
assert response.status_code == 400
assert response.json()["errors"] == {"status": [MERGED_STATUS_LOCKED]}
_unchanged(merged)
def test_move_of_an_unmerged_ticket_goes_through(self, admin_client, plain):
response = admin_client.patch(
f"/api/cases/{plain.id}/move/", {"status": "Assigned"}, format="json"
)
assert response.status_code == 200, response.content
plain.refresh_from_db()
assert plain.status == "Assigned"
class TestBulkUpdate:
def test_merged_ticket_reported_and_left_alone(self, admin_client, merged, plain):
response = admin_client.post(
"/api/cases/bulk/update/",
{"ids": [str(merged.id), str(plain.id)], "fields": {"status": "Pending"}},
format="json",
)
assert response.status_code == 200, response.content
body = response.json()
assert body["updated"] == 1
by_id = {r["id"]: r for r in body["results"]}
assert by_id[str(merged.id)] == {
"id": str(merged.id),
"status": "merged",
"detail": MERGED_STATUS_LOCKED,
}
assert by_id[str(plain.id)]["status"] == "updated"
_unchanged(merged)
plain.refresh_from_db()
assert plain.status == "Pending"
def test_a_non_status_bulk_edit_reaches_a_merged_ticket(self, admin_client, merged):
response = admin_client.post(
"/api/cases/bulk/update/",
{"ids": [str(merged.id)], "fields": {"priority": "Urgent"}},
format="json",
)
assert response.json()["results"] == [
{"id": str(merged.id), "status": "updated"}
]
merged.refresh_from_db()
assert merged.priority == "Urgent" and merged.status == "Duplicate"
class TestCloseWithChildren:
def test_closing_a_merged_ticket_is_refused(self, admin_client, merged):
response = admin_client.post(
f"/api/cases/{merged.id}/close-with-children/",
{"cascade": False},
format="json",
)
assert response.status_code == 400
assert response.json()["errors"] == {"status": [MERGED_STATUS_LOCKED]}
_unchanged(merged)
def test_the_cascade_passes_over_a_merged_child(
self, admin_client, admin_user, org_a
):
parent = _case(org_a, admin_user, "Incident")
open_child = _case(org_a, admin_user, "Open child", parent=parent)
survivor = _case(org_a, admin_user, "Survivor")
merged_child = _case(org_a, admin_user, "Merged child", parent=parent)
assert (
admin_client.post(
f"/api/cases/{merged_child.id}/merge/{survivor.id}/"
).status_code
== 200
)
response = admin_client.post(
f"/api/cases/{parent.id}/close-with-children/",
{"cascade": True},
format="json",
)
assert response.status_code == 200, response.content
assert response.json()["cascaded_case_ids"] == [str(open_child.id)]
merged_child.refresh_from_db()
assert merged_child.status == "Duplicate"
open_child.refresh_from_db()
assert open_child.status == "Closed"
class TestReopen:
"""A merged ticket left Closed by an edit before this rule existed is not
reopened by a customer reply; an ordinary closed ticket still is."""
def _closed(self, case, **extra):
with rls_org(case.org):
Case.objects.filter(pk=case.pk).update(
status="Closed",
closed_on=timezone.localdate() - timedelta(days=1),
**extra,
)
case.refresh_from_db()
return case
def _customer_reply(self, case):
with rls_org(case.org):
Comment.objects.create(
comment="Still broken",
content_type=ContentType.objects.get_for_model(Case),
object_id=case.pk,
commented_by=None,
is_internal=False,
org=case.org,
)
case.refresh_from_db()
def test_merged_and_closed_stays_closed(self, admin_user, org_a):
survivor = _case(org_a, admin_user, "Survivor")
legacy = self._closed(
_case(org_a, admin_user, "Legacy merged"), merged_into=survivor
)
self._customer_reply(legacy)
assert legacy.status == "Closed"
def test_unmerged_and_closed_reopens(self, admin_user, org_a):
closed = self._closed(_case(org_a, admin_user, "Closed"))
self._customer_reply(closed)
assert closed.status != "Closed"

View file

@ -52,7 +52,7 @@ class TestDetail:
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
assert user_client.get(f"/api/cases/{parent.id}/").status_code == 404
resp = user_client.get(f"/api/cases/{child.id}/")
assert resp.status_code == 200, resp.content

View file

@ -65,14 +65,14 @@ class TestTreeAccess:
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
# The ticket detail GET answers a hidden in-org ticket 404 too.
assert resp.status_code == 404
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
assert user_client.get(f"/api/cases/{root.id}/").status_code == 404
def test_watcher_may_read_the_tree(
self, user_client, admin_user, user_profile, org_a
@ -146,10 +146,12 @@ class TestLinkAccess:
)
def test_member_cannot_unlink_a_ticket_they_cannot_write(
self, user_client, admin_user, org_a
self, user_client, admin_user, user_profile, org_a
):
parent = _case(org_a, admin_user, "Admin parent")
child = _case(org_a, admin_user, "Admin child", parent=parent)
# Watching grants read, not write, so the refusal is the 403.
_watch(child, user_profile)
resp = user_client.post(
f"/api/cases/{child.id}/link/", {"parent_id": None}, format="json"
)

View file

@ -342,7 +342,7 @@ class TestLinkedCasesIsNotAWayIntoCases:
def test_the_member_really_cannot_open_that_case(self, user_client, hidden_case):
"""The premise. Without this the rest proves nothing."""
response = user_client.get(f"/api/cases/{hidden_case.pk}/")
assert response.status_code == status.HTTP_403_FORBIDDEN
assert response.status_code == status.HTTP_404_NOT_FOUND
def test_member_may_not_link_an_article_to_it(
self, user_client, hidden_case, member_article
@ -352,7 +352,7 @@ class TestLinkedCasesIsNotAWayIntoCases:
{"solution_id": str(member_article.pk)},
format="json",
)
assert response.status_code == status.HTTP_403_FORBIDDEN
assert response.status_code == status.HTTP_404_NOT_FOUND
assert hidden_case.solutions.count() == 0
def test_member_may_not_unlink_from_it(
@ -360,7 +360,7 @@ class TestLinkedCasesIsNotAWayIntoCases:
):
hidden_case.solutions.add(member_article)
response = user_client.delete(_unlink(hidden_case.pk, member_article.pk))
assert response.status_code == status.HTTP_403_FORBIDDEN
assert response.status_code == status.HTTP_404_NOT_FOUND
assert hidden_case.solutions.count() == 1
def test_linked_cases_hides_the_case_from_the_member(

View file

@ -0,0 +1,88 @@
"""Logging time and starting a timer take the ticket's write rule.
Listing a ticket's time and reading its summary need read access. Logging time
and starting a timer are work on the ticket, so they take the rule replying
takes (`comment_permission` on the detail payload): an assignee or creator may,
a watcher (who may read) gets 403, and a member who cannot open the ticket gets
the 404 a missing ticket gets. Stopping stays the entry owner's.
"""
from datetime import timedelta
import pytest
from django.utils import timezone
from cases.models import Case, CaseWatcher, TimeEntry
@pytest.fixture
def ticket(admin_user, org_a):
return Case.objects.create(
name="Printer on fire",
status="New",
priority="Normal",
org=org_a,
created_by=admin_user,
)
def _log(client, case):
end = timezone.now() - timedelta(minutes=5)
return client.post(
f"/api/cases/{case.id}/time-entries/",
{
"started_at": (end - timedelta(minutes=30)).isoformat(),
"ended_at": end.isoformat(),
"description": "Replaced the toner",
},
format="json",
)
def _start(client, case):
return client.post(f"/api/cases/{case.id}/time-entries/start/", {}, format="json")
WRITES = [_log, _start]
@pytest.mark.parametrize("write", WRITES)
def test_an_assignee_may(write, user_client, user_profile, ticket):
ticket.assigned_to.add(user_profile)
assert write(user_client, ticket).status_code == 201
assert TimeEntry.objects.filter(case=ticket, profile=user_profile).count() == 1
@pytest.mark.parametrize("write", WRITES)
def test_a_watcher_reads_but_may_not_write(
write, user_client, user_profile, ticket, org_a
):
CaseWatcher.objects.create(case=ticket, profile=user_profile, org=org_a)
assert user_client.get(f"/api/cases/{ticket.id}/time-entries/").status_code == 200
assert user_client.get(f"/api/cases/{ticket.id}/time-summary/").status_code == 200
# The detail payload tells the clients the same thing.
detail = user_client.get(f"/api/cases/{ticket.id}/").json()
assert detail["comment_permission"] is False
assert write(user_client, ticket).status_code == 403
assert not TimeEntry.objects.filter(case=ticket).exists()
@pytest.mark.parametrize("write", WRITES)
def test_a_member_who_cannot_open_it_gets_404(write, user_client, ticket):
assert write(user_client, ticket).status_code == 404
assert not TimeEntry.objects.filter(case=ticket).exists()
def test_the_owner_still_stops_their_timer_after_losing_the_ticket(
user_client, user_profile, ticket
):
"""Stopping is not gated on the ticket: refusing it would strand the one
running timer a person may have, org-wide."""
ticket.assigned_to.add(user_profile)
entry_id = _start(user_client, ticket).json()["id"]
ticket.assigned_to.remove(user_profile)
response = user_client.post(f"/api/time-entries/{entry_id}/stop/")
assert response.status_code == 200, response.content
assert TimeEntry.objects.get(id=entry_id).ended_at is not None

View file

@ -148,6 +148,7 @@ class TestListEndpoint:
org_a,
):
case = _make_case(org_a, admin_user)
case.assigned_to.add(user_profile)
_entry(case, admin_profile, ended_at=timezone.now())
_entry(case, user_profile, ended_at=timezone.now())
resp = user_client.get(f"/api/cases/{case.id}/time-entries/")

View file

@ -8,6 +8,16 @@ Case-scoped:
* ``POST /api/cases/<pk>/time-entries/start/``: start a running timer (409 if one active)
* ``GET /api/cases/<pk>/time-summary/``: totals + by-profile breakdown
All four need read access to the ticket (`get_case_or_404`), and a ticket the
caller may not open is a 404. They used to check the org alone, so any member
could log time on, start a timer against, or read the per-person breakdown of
a ticket the detail view refuses them. Logging time and starting a timer are
work on the ticket, so they also take its write rule (the one replying takes,
`comment_permission` on the detail payload): a watcher reads, and gets 403.
Stopping a timer is not gated on the ticket: it is the entry owner's (or an
admin's) and only ends their own clock, and refusing it to someone since
unassigned would strand their one running timer.
Entry-scoped (registered at the project root under ``/api/time-entries/``):
* ``POST /api/time-entries/<pk>/stop/``: stop a running timer
@ -23,7 +33,6 @@ from datetime import datetime, timedelta
from django.db import IntegrityError, transaction
from django.db.models import Sum
from django.shortcuts import get_object_or_404
from django.utils import timezone
from rest_framework import status
from rest_framework.permissions import IsAuthenticated
@ -31,7 +40,8 @@ from rest_framework.response import Response
from rest_framework.views import APIView
from cases import time_reports
from cases.models import Case, TimeEntry
from cases.access import assert_case_write_access, get_case_or_404
from cases.models import TimeEntry
from cases.serializer import (
TimeEntryCreateSerializer,
TimeEntrySerializer,
@ -128,14 +138,15 @@ class TimeEntryListCreateView(APIView):
permission_classes = (IsAuthenticated, HasOrgContext)
def get(self, request, pk):
case = get_object_or_404(Case, id=pk, org=request.profile.org)
case = get_case_or_404(request.profile, pk)
entries = (
_visible_entry_qs(request.profile).filter(case=case).order_by("-started_at")
)
return Response(TimeEntrySerializer(entries, many=True).data)
def post(self, request, pk):
case = get_object_or_404(Case, id=pk, org=request.profile.org)
case = get_case_or_404(request.profile, pk)
assert_case_write_access(request.profile, case)
serializer = TimeEntryCreateSerializer(data=request.data)
serializer.is_valid(raise_exception=True)
try:
@ -160,7 +171,8 @@ class TimeEntryStartView(APIView):
@transaction.atomic
def post(self, request, pk):
case = get_object_or_404(Case, id=pk, org=request.profile.org)
case = get_case_or_404(request.profile, pk)
assert_case_write_access(request.profile, case)
# Reject if this profile already has a running timer (anywhere). The
# one_active_timer_per_profile partial unique would also catch this
@ -202,7 +214,7 @@ class TimeSummaryView(APIView):
permission_classes = (IsAuthenticated, HasOrgContext)
def get(self, request, pk):
case = get_object_or_404(Case, id=pk, org=request.profile.org)
case = get_case_or_404(request.profile, pk)
qs = case.time_entries.filter(ended_at__isnull=False)
total = qs.aggregate(total=Sum("duration_minutes"))["total"] or 0
billable = (

View file

@ -21,6 +21,7 @@ from rest_framework.permissions import IsAuthenticated
from rest_framework.response import Response
from rest_framework.views import APIView
from cases.access import has_case_read_access
from cases.merge_views import _can_merge
from cases.models import Case, EmailMessage
from cases.serializer import CaseSerializer, parent_access_context
@ -50,9 +51,9 @@ class CaseUnmergeView(APIView):
org = request.profile.org
with transaction.atomic():
try:
source = Case.objects.select_for_update().get(id=pk, org=org)
except Case.DoesNotExist:
source = Case.objects.select_for_update().filter(id=pk, org=org).first()
# A ticket the caller may not open answers as a missing one does.
if source is None or not has_case_read_access(request.profile, source):
return Response(
{"error": True, "errors": "Ticket not found."},
status=status.HTTP_404_NOT_FOUND,

View file

@ -6,7 +6,6 @@ from django.contrib.contenttypes.models import ContentType
from django.core.exceptions import ValidationError as DjangoValidationError
from django.db import transaction
from django.db.models import Q
from django.http import Http404
from django.utils import timezone
from drf_spectacular.utils import (
extend_schema,
@ -23,7 +22,6 @@ from accounts.serializer import AccountPickerSerializer
from cases import swagger_params
from cases.access import (
assert_case_delete_access,
assert_case_read_access,
assert_case_write_access,
get_case_or_404,
has_case_write_access,
@ -47,6 +45,7 @@ from cases.signals import route_after_relations
from cases.solution_serializers import SolutionSerializer
from cases.tasks import send_email_to_assigned_user
from common.custom_fields import validate_payload as validate_custom_fields_payload
from common.lookups import get_on_visible_record_or_404
from common.models import (
Activity,
Attachments,
@ -445,7 +444,7 @@ class CaseDetailView(APIView):
model = Case
def get_object(self, pk):
"""A case in the requester's org, or 404.
"""A case the requester may open, or 404 (see `get_case_or_404`).
This used to return ``None`` for a missing case and leave every caller
to notice. Only `get` did; `put`, `patch` and `delete` went straight
@ -596,11 +595,11 @@ class CaseDetailView(APIView):
},
)
def get(self, request, pk, format=None):
# Authorised by the lookup itself, 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.
self.cases = self.get_object(pk=pk)
# 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
@ -948,17 +947,21 @@ class CaseCommentView(APIView):
permission_classes = (IsAuthenticated, HasOrgContext)
def get_object(self, pk):
"""Org-scoped already; `.get()` was the problem. A comment id that
does not exist raised DoesNotExist and answered 500 instead of 404."""
try:
comment = self.model.objects.filter(
pk=pk, org=self.request.profile.org
).first()
except (DjangoValidationError, ValueError):
raise Http404("No such comment.")
if comment is None:
raise Http404("No such comment.")
return comment
"""A comment on a ticket the caller may open, or 404.
It used to be fetched from the whole org, across every module's
comments, and then refused with 403 when the caller was not its author:
an oracle for comments on tickets (and leads and deals) they cannot
open, and a way for an author who had lost the ticket to keep editing
and deleting there. A comment on a hidden ticket, or on another
module's record, now answers exactly like an id that does not exist.
"""
return get_on_visible_record_or_404(
self.model,
pk,
self.request.profile.org,
visible_cases_qs(self.request.profile),
)
@extend_schema(
tags=["Cases"],
@ -1110,14 +1113,12 @@ class CaseAttachmentView(APIView):
The same one-line lookup bug is still open in `leads`, `tasks` and
`opportunity`.
"""
try:
self.object = self.model.objects.filter(
pk=pk, org=request.profile.org
).first()
except (DjangoValidationError, ValueError):
raise Http404("No such attachment.")
if self.object is None:
raise Http404("No such attachment.")
# On a ticket the caller may open, or the 404 a missing id gets: an
# org-wide fetch answered 403 for an attachment on a hidden ticket or
# on any other module's record, which confirmed it existed.
self.object = get_on_visible_record_or_404(
self.model, pk, request.profile.org, visible_cases_qs(request.profile)
)
if (
is_org_admin(request.profile)
@ -1237,7 +1238,6 @@ class CaseActivityListView(APIView, LimitOffsetPagination):
)
def get(self, request, pk):
case = get_case_or_404(request.profile, pk)
assert_case_read_access(request.profile, case)
queryset = Activity.objects.filter(
entity_type="Case",

View file

@ -124,3 +124,28 @@ def duplicate_refusal(old_status, new_status):
if new_status == "Duplicate" and old_status != "Duplicate":
return {"status": DUPLICATE_BY_MERGE_ONLY}
return None
MERGED_STATUS_LOCKED = (
"This ticket is merged into another. Unmerge it first to change its status."
)
def merged_status_refusal(case, new_status):
"""Refuse changing the status of a merged ticket.
A merged ticket (``merged_into`` set) is Duplicate because the merge made
it so, and its comments, attachments and email thread now live on the
ticket it was merged into. Edited to another status it kept ``merged_into``,
so it was open and merged at once: back on the list and the board, still
redirecting to the target, and holding none of its own history. Unmerge is
the way back; it restores the status the ticket had before the merge.
Re-sending the status the ticket already has is not a change, so a full
PUT that edits only the description still goes through.
Returns ``{"status": message}`` or None, the shape ``close_refusal`` uses.
"""
if case is not None and case.merged_into_id and new_status != case.status:
return {"status": MERGED_STATUS_LOCKED}
return None

View file

@ -21,6 +21,7 @@ import logging
from django.db import models
from common.base import BaseModel
from common.request_meta import client_ip
logger = logging.getLogger("security.audit")
@ -47,6 +48,9 @@ class SecurityAuditLog(BaseModel):
("MEMBERSHIP_REVOKED", "Membership Revoked"),
("SUSPICIOUS_ACTIVITY", "Suspicious Activity"),
("SAMPLE_DATA_CLEARED", "Vertical Pack Sample Data Cleared"),
("WEBHOOK_PAUSED", "Webhook Paused"),
("WEBHOOK_REENABLED", "Webhook Re-enabled"),
("WEBHOOK_CHANGED", "Webhook Destination Changed"),
)
event_type = models.CharField(max_length=50, choices=EVENT_TYPES, db_index=True)
@ -107,15 +111,10 @@ class AuditLogger:
if not request:
return {}
# Get IP address
ip = request.META.get("HTTP_X_FORWARDED_FOR")
if ip:
ip = ip.split(",")[0].strip()
else:
ip = request.META.get("REMOTE_ADDR")
# Never the first X-Forwarded-For entry: the caller writes that, and
# admins read this column in the audit log viewer as fact.
return {
"ip_address": ip,
"ip_address": client_ip(request),
"user_agent": request.META.get("HTTP_USER_AGENT", "")[:500],
"request_path": request.path[:500],
"request_method": request.method,
@ -313,6 +312,51 @@ class AuditLogger:
request=request,
)
def webhook_paused(self, endpoint, reason, creator=None, request=None):
"""Log a webhook paused because its creator lost the standing to own
it (see `webhooks.ownership`). `user` is that creator, or None when
their user row is being deleted; `creator_id` survives either way.
The URL is left out on purpose: hook URLs often carry a secret.
"""
self._log(
"WEBHOOK_PAUSED",
user=creator,
org=endpoint.org,
description=f"Webhook paused: {reason}",
metadata={
"endpoint_id": str(endpoint.id),
"creator_id": (
str(endpoint.created_by_id) if endpoint.created_by_id else None
),
"pause_reason": reason,
},
request=request,
)
def webhook_taken_over(
self, user, endpoint, previous_creator_id, changed, request=None
):
"""Log an admin becoming the creator of a webhook, which happens when
they turn it back on or change what it sends or where (`changed`
names the fields, `is_active` for a re-enable). `previous_creator_id`
is who answered for it before. See `webhooks.views`.
"""
self._log(
"WEBHOOK_REENABLED" if changed == ["is_active"] else "WEBHOOK_CHANGED",
user=user,
org=endpoint.org,
description=f"Webhook taken over: {', '.join(changed)}",
metadata={
"endpoint_id": str(endpoint.id),
"previous_creator_id": (
str(previous_creator_id) if previous_creator_id else None
),
"changed": list(changed),
},
request=request,
)
def suspicious_activity(self, user, org, activity_type, details, request=None):
"""Log suspicious activity for security review."""
self._log(

View file

@ -40,3 +40,24 @@ def get_scoped_or_404(model, pk, org, **extra):
if obj is None:
raise Http404(f"No such {model._meta.verbose_name}.")
return obj
def get_on_visible_record_or_404(model, pk, org, visible):
"""A comment or attachment by pk, only if it hangs off a record in ``visible``.
``model`` is `Comment` or `Attachments`, one generic table per kind shared
by every module. ``visible`` is the module's read-rule queryset (for
example `visible_leads_qs(profile, user)`), so a comment on a record the
caller cannot open, or on another module's record, raises the same 404 as
an id that does not exist. Checking the author after an org-wide fetch
answered 403 there, which confirmed the id was real.
"""
from django.contrib.contenttypes.models import ContentType
return get_scoped_or_404(
model,
pk,
org,
content_type=ContentType.objects.get_for_model(visible.model),
object_id__in=visible.values("id"),
)

View file

@ -232,7 +232,7 @@ class GetProfileAndOrg:
# Validate user membership in the org
try:
profile = Profile.objects.select_related("org").get(
profile = Profile.objects.select_related("org", "user").get(
user_id=user_id, org_id=org_id, is_active=True
)
request.profile = profile

View file

@ -0,0 +1,139 @@
# Generated by Django 6.1.1 on 2026-09-26 13:17
import uuid
import django.db.models.deletion
import django.db.models.functions.text
from django.conf import settings
from django.db import connection, migrations, models
from common.rls import get_disable_policy_sql, get_enable_policy_sql
# Registered in common/rls/__init__.py:ORG_SCOPED_TABLES.
TABLE = "saved_view"
def enable_rls(apps, schema_editor):
if connection.vendor != "postgresql":
return
with connection.cursor() as cursor:
cursor.execute(get_enable_policy_sql(TABLE))
def disable_rls(apps, schema_editor):
if connection.vendor != "postgresql":
return
with connection.cursor() as cursor:
cursor.execute(get_disable_policy_sql(TABLE))
class Migration(migrations.Migration):
# RLS policy creation cannot run inside an atomic block.
atomic = False
dependencies = [
("common", "0043_org_help_center"),
]
operations = [
migrations.CreateModel(
name="SavedView",
fields=[
(
"created_at",
models.DateTimeField(auto_now_add=True, verbose_name="Created At"),
),
(
"updated_at",
models.DateTimeField(
auto_now=True, verbose_name="Last Modified At"
),
),
(
"id",
models.UUIDField(
db_index=True,
default=uuid.uuid4,
editable=False,
primary_key=True,
serialize=False,
unique=True,
),
),
(
"module",
models.CharField(
choices=[
("leads", "Leads"),
("contacts", "Contacts"),
("accounts", "Accounts"),
("opportunities", "Deals"),
("cases", "Tickets"),
("invoices", "Invoices"),
],
max_length=20,
),
),
("name", models.CharField(max_length=100)),
("filters", models.JSONField(blank=True, default=dict)),
(
"created_by",
models.ForeignKey(
blank=True,
null=True,
on_delete=django.db.models.deletion.SET_NULL,
related_name="%(class)s_created_by",
to=settings.AUTH_USER_MODEL,
verbose_name="Created By",
),
),
(
"org",
models.ForeignKey(
help_text="Organization this record belongs to",
on_delete=django.db.models.deletion.CASCADE,
related_name="%(class)s_set",
to="common.org",
),
),
(
"profile",
models.ForeignKey(
on_delete=django.db.models.deletion.CASCADE,
related_name="saved_views",
to="common.profile",
),
),
(
"updated_by",
models.ForeignKey(
blank=True,
null=True,
on_delete=django.db.models.deletion.SET_NULL,
related_name="%(class)s_updated_by",
to=settings.AUTH_USER_MODEL,
verbose_name="Last Modified By",
),
),
],
options={
"db_table": "saved_view",
"ordering": ("name",),
"indexes": [
models.Index(
fields=["org", "-created_at"],
name="saved_view_org_id_77a76d_idx",
)
],
"constraints": [
models.UniqueConstraint(
models.F("profile"),
models.F("module"),
django.db.models.functions.text.Lower("name"),
name="saved_view_name_ci_unique",
)
],
},
),
migrations.RunPython(enable_rls, reverse_code=disable_rls),
]

View file

@ -0,0 +1,38 @@
# Generated by Django 6.1.1 on 2026-09-26 14:52
from django.db import migrations, models
class Migration(migrations.Migration):
dependencies = [
("common", "0044_saved_view"),
]
operations = [
migrations.AlterField(
model_name="securityauditlog",
name="event_type",
field=models.CharField(
choices=[
("LOGIN_SUCCESS", "Login Success"),
("LOGIN_FAILURE", "Login Failure"),
("LOGOUT", "Logout"),
("ORG_SWITCH", "Organization Switch"),
("TOKEN_REFRESH", "Token Refresh"),
("TOKEN_REVOKED", "Token Revoked"),
("PERMISSION_DENIED", "Permission Denied"),
("CROSS_ORG_ATTEMPT", "Cross-Org Access Attempt"),
("API_KEY_USED", "API Key Used"),
("API_KEY_INVALID", "Invalid API Key"),
("MEMBERSHIP_REVOKED", "Membership Revoked"),
("SUSPICIOUS_ACTIVITY", "Suspicious Activity"),
("SAMPLE_DATA_CLEARED", "Vertical Pack Sample Data Cleared"),
("WEBHOOK_PAUSED", "Webhook Paused"),
("WEBHOOK_REENABLED", "Webhook Re-enabled"),
("WEBHOOK_CHANGED", "Webhook Destination Changed"),
],
db_index=True,
max_length=50,
),
),
]

View file

@ -355,12 +355,15 @@ class Profile(BaseModel):
@property
def is_admin(self):
"""Alias kept for the API response and for existing callers.
"""`common.permissions.is_org_admin` for this profile.
Reads `role`, not the column, so it answers the same as
`is_org_admin` even on an unsaved instance.
Delegates rather than restating the rule, so it counts a superuser's
profile as an admin too, and reads `role` rather than the column, so it
answers correctly on an unsaved instance.
"""
return self.role == "ADMIN"
from common.permissions import is_org_admin
return is_org_admin(self)
@property
def user_details(self):
@ -1208,3 +1211,47 @@ class PackApplication(BaseOrgModel):
def __str__(self):
return f"{self.pack_id} v{self.pack_version} → {self.org.name}"
class SavedView(BaseOrgModel):
"""A profile's named filters for one list, to put back with one tap (G29).
Private to ``profile``: every read and write is filtered on it as well as on
the org, and another profile's id answers 404. ``filters`` holds the list's
own query parameters as ``{param: [value, ...]}``; which parameters a list
takes, and how they are checked, lives in ``common/saved_views.py``.
"""
MODULE_CHOICES = (
("leads", "Leads"),
("contacts", "Contacts"),
("accounts", "Accounts"),
("opportunities", "Deals"),
("cases", "Tickets"),
("invoices", "Invoices"),
)
profile = models.ForeignKey(
"common.Profile", on_delete=models.CASCADE, related_name="saved_views"
)
module = models.CharField(max_length=20, choices=MODULE_CHOICES)
name = models.CharField(max_length=100)
filters = models.JSONField(default=dict, blank=True)
class Meta:
db_table = "saved_view"
ordering = ("name",)
indexes = [
models.Index(fields=["org", "-created_at"]),
]
constraints = [
# Case-insensitive, so "Hot leads" and "hot leads" cannot both
# appear in one menu. Leading with the profile makes this the
# index the per-profile lists read through as well.
models.UniqueConstraint(
"profile", "module", Lower("name"), name="saved_view_name_ci_unique"
),
]
def __str__(self):
return f"{self.module}: {self.name}"

View file

@ -272,6 +272,18 @@ def _validate_pipeline(
valid_stage_types=valid_stage_types,
valid_maps_to_status=valid_maps_to_status,
)
# A board move into such a stage would set Duplicate without a merge.
# `CaseStageSerializer` refuses it for a stage made through the API;
# a pack writes stages without that serializer, so it is refused here.
if pipeline_key == "case_pipeline" and stage.get("maps_to_status") == (
"Duplicate"
):
from cases.workflow import DUPLICATE_BY_MERGE_ONLY
raise PackValidationError(
f"{where}.stages[{i}]: maps_to_status 'Duplicate' is not "
f"allowed. {DUPLICATE_BY_MERGE_ONLY}"
)
def _validate_custom_field(where: str, field: dict) -> None:

View file

@ -9,38 +9,48 @@ from rest_framework import permissions
def is_org_admin(profile):
"""Whether ``profile`` administers its org. ``role`` is the only source.
"""Whether ``profile`` administers its org: ``role == "ADMIN"``, or the
profile belongs to a Django superuser.
Superusers are org admins everywhere (owner decision, 1.11.0). Before that,
this read ``role`` only while dozens of call sites added ``or
request.user.is_superuser`` by hand and the rest did not, so a superuser
holding the USER role could change a settings page at one endpoint and was
refused at the next, and both clients showed it read-only. One rule, here.
Membership is still required. This takes a ``Profile``, which is one
user's membership of one org, and every request's profile is resolved by
the middleware from an active ``Profile`` row for the org in the signed
token. A superuser with no profile in an org gets no profile there, so
this function is never asked about that org. ``is_superuser`` itself is
not writable through any API serializer; it is granted with
``createsuperuser`` or the Django admin.
A plain function and not only the ``IsOrgAdmin`` class below, because most
callers are views that read wide and write narrow: the same endpoint is
open to every member on GET and admin-only on POST, so the check has to
happen inside the method rather than in ``permission_classes``.
That is why this rule had been written out nine times, as a private
``_is_admin`` in nine modules, in three different spellings. Five of them
raised ``AttributeError`` on a ``None`` profile where two returned
``False``. All nine agreed for a real profile, so nothing was broken; one
edit that missed eight copies is how that stops being true.
**This deliberately does not consult ``is_organization_admin``.** That
column mirrors ``role`` (``Profile.save`` derives it) and is not an input
anywhere; it used to be, and an admin could ``PATCH`` a colleague to
``{"is_organization_admin": true, "role": "USER"}`` and grant an admin the
UI could neither show nor revoke. The API field of the same name is
computed from this function (see ``common.serializer``), not read from the
column.
**This deliberately does not consult ``is_organization_admin``.** ``ROLES``
has exactly two members, so that column and ``role == "ADMIN"`` are two
spellings of one binary fact, and the backend used to read them
inconsistently: 82 checks looked only at ``role`` while 43 also accepted
the flag, with both spellings appearing inside the same file. The flag is
the dangerous half of that pair, because no frontend surface displays or
sets it: an admin could ``PATCH`` a colleague to
``{"is_organization_admin": true, "role": "USER"}`` and grant admin over
accounts, contacts, team management and the org record itself, invisibly
and irrevocably from the UI. ``Profile.save`` now derives the column from
``role`` so the two cannot disagree, and ``ProfileSerializer`` no longer
accepts it as an input.
``is_superuser`` must be literally ``True``. A test double or a partially
loaded object answering some other truthy value is not an admin.
``None`` is not an admin. A view with no org context has no profile, and
answering ``False`` gives it a clean 403 instead of a 500.
"""
if profile is None:
return False
return getattr(profile, "role", None) == "ADMIN"
if getattr(profile, "role", None) == "ADMIN":
return True
user = getattr(profile, "user", None)
return getattr(user, "is_superuser", False) is True
def can_mass_import(profile):
@ -94,7 +104,8 @@ class HasOrgContext(permissions.BasePermission):
class IsOrgAdmin(permissions.BasePermission):
"""
Permission class that requires user to be an organization admin.
Permission class that requires an org admin, as ``is_org_admin`` defines it
(ADMIN role, or a superuser's profile in this org).
Usage:
class AdminOnlyView(APIView):

View file

@ -1,51 +1,53 @@
"""Request metadata derived server-side.
Shared by the views that record it and the throttles that bucket on it, so both
agree on what "the client" means. A throttle keyed on one definition of the
client and a stored value using another is a bug that only shows up during an
incident.
`common/audit_log.py` carries an unvalidated inline version of the same idea.
It is deliberately left alone: changing it is outside the scope of the web
forms work and would need its own tests.
Shared by every view that records a client IP (the security audit log, web form
submissions, estimate acceptance, magic-link and portal sign-in tokens) and by
the throttles that bucket on it, so all of them agree on what "the client"
means. One function: a second definition is how one of them ended up trusting a
header the caller writes.
"""
from django.core.exceptions import ValidationError
from django.core.validators import validate_ipv46_address
from rest_framework.settings import api_settings
REFERER_MAX_LENGTH = 512
def _valid_ip(candidate):
"""`candidate` if it is an IPv4 or IPv6 address, else None. Callers write
the result into a GenericIPAddressField, and Django does not run field
validators on save(), so junk would reach the `inet` column as a 500."""
try:
validate_ipv46_address(candidate)
except ValidationError:
return None
return candidate
def client_ip(request):
"""Best-effort originating client IP, or None when nothing validates.
"""The client IP as far as the proxies we run can vouch for it, or None.
A reverse proxy makes the socket peer the proxy rather than the visitor, so
every visitor would otherwise collapse into a single throttle bucket.
`X-Forwarded-For` carries the original client as its first entry.
Each proxy appends the address it received the request from to
`X-Forwarded-For`, so only the last `NUM_PROXIES` entries were written by
infrastructure; anything to their left is whatever the caller sent, and
trusting it let a caller pick their own throttle bucket and their own
recorded address. With `REST_FRAMEWORK["NUM_PROXIES"] = n` the answer is
the n-th entry from the right, the address the outermost proxy saw. Unset
or 0 means no proxy is trusted: the header is ignored and the socket peer
(`REMOTE_ADDR`) is the answer. DRF's own setting, so its `get_ident` and
this cannot disagree.
That header is submitter-controlled for anything that can reach the
endpoint directly, so the result is informational: it is stored for triage
and used to bucket the per-IP throttle, and it is never an input to an
authorization decision. The global throttle is the control a forged header
cannot evade.
Every candidate is validated before it is returned, because the caller
writes this into a GenericIPAddressField and Django does not run field
validators on save().
Behind a proxy with `NUM_PROXIES` unset, every visitor is the proxy, so
the per-IP throttles become one shared bucket. Production sets it.
"""
remote = request.META.get("REMOTE_ADDR", "")
num_proxies = api_settings.NUM_PROXIES or 0
forwarded = request.META.get("HTTP_X_FORWARDED_FOR", "")
candidates = [part.strip() for part in forwarded.split(",") if part.strip()]
candidates.append(request.META.get("REMOTE_ADDR", ""))
for candidate in candidates:
if not candidate:
continue
try:
validate_ipv46_address(candidate)
except ValidationError:
continue
return candidate
return None
if num_proxies <= 0 or not forwarded:
return _valid_ip(remote)
hops = [part.strip() for part in forwarded.split(",")]
return _valid_ip(hops[-min(num_proxies, len(hops))])
def referer(request):

View file

@ -112,10 +112,10 @@ ORG_SCOPED_TABLES = [
# a `logger.error`, so the audit trail was silently absent in exactly the
# deployments configured correctly, for exactly the hostile events. It was
# invisible because the dev database user is a superuser and CI ran SQLite.
# `common/0036` drops the policies. Nothing outside a test reads the model,
# and `common/tests/test_audit_log_not_exposed.py` keeps it that way: if an
# endpoint ever needs to serve these rows, it must do the org filtering in
# the ORM, because there is no policy underneath it.
# `common/0036` drops the policies. The one endpoint that serves the rows,
# `common/views/audit_log_views.py`, does the org filtering in the ORM,
# because there is no policy underneath it, and
# `common/tests/test_audit_log_not_exposed.py` fails on any new reader.
#
# Notifications
"notification",
@ -148,6 +148,9 @@ ORG_SCOPED_TABLES = [
# context from the org it was queued for. See webhooks/tasks.py.
"webhook_endpoint",
"webhook_delivery",
# Saved list views (G29). Private to one profile as well; the views filter
# on it, this keeps another org's rows out even if one forgets.
"saved_view",
# Programmatic API access
# NOTE: personal_access_token is intentionally NOT RLS-protected. It is an
# auth-bootstrap table (looked up by token_hash before any tenant context

View file

@ -0,0 +1,303 @@
"""Saved list views (G29): which lists can have one, and what a view may hold.
A saved view is a name and a set of list query parameters, stored per profile
so a person can put a filtered list back with one tap. Only the six lists that
also export to CSV take one.
**What a view may hold is what its list reads.** Each entry in :data:`LISTS`
pairs the function the list view (and its CSV export) builds its queryset
with, and the query parameters that function reads. A view is checked two
ways before it is stored:
* every key is one of those parameters, or a ``cf_<key>`` custom-field filter,
which every one of these lists takes. Anything else is refused, so a view
cannot become a place to park arbitrary data;
* the values are handed to that same function, so a malformed id, date or
number is refused with the list's own 400 now, rather than taking the list
down every time the view is applied.
The parameter sets are written out rather than discovered, so a change to what
a view can hold shows up in a diff. ``common/tests/test_saved_views.py`` reads
each list function with a recording mapping and fails when the two disagree,
in either direction.
"""
import re
from django.db import IntegrityError, transaction
from django.http import QueryDict
from rest_framework import serializers
from accounts.views import account_list_queryset
from cases.views import case_list_queryset
from common.models import SavedView
from contacts.views import contact_list_queryset
from invoices.api_views import filter_invoices
from invoices.permissions import visible_invoices_qs
from leads.views.lead_views import lead_list_queryset
from opportunity.views.opportunity_views import deal_list_queryset
MAX_VIEWS_PER_LIST = 25
MAX_PARAMS = 30
MAX_VALUES_PER_PARAM = 50
MAX_VALUE_LENGTH = 200
CUSTOM_FIELD_PARAM = re.compile(r"^cf_[A-Za-z0-9_]{1,100}$")
_DATES = ("created_at__gte", "created_at__lte")
LISTS = {
"leads": (
lambda request, params: lead_list_queryset(
request.profile, request.user, params
),
frozenset(
{
"name",
"salutation",
"source",
"assigned_to",
"status",
"tags",
"city",
"email",
"rating",
"search",
*_DATES,
"close_date__gte",
"close_date__lte",
"next_follow_up",
"open",
}
),
),
"contacts": (
lambda request, params: contact_list_queryset(request.profile, params)[1],
frozenset(
{
"name",
"city",
"phone",
"email",
"assigned_to",
"tags",
"search",
*_DATES,
"is_active",
}
),
),
"accounts": (
lambda request, params: account_list_queryset(
request.profile, request.user, params
),
frozenset(
{
"name",
"city",
"industry",
"tags",
"assigned_to",
"search",
*_DATES,
"is_active",
}
),
),
"opportunities": (
lambda request, params: deal_list_queryset(
request.profile, request.user, params
),
frozenset(
{
"name",
"account",
"pipeline",
"stage",
"lead_source",
"tags",
"assigned_to",
"search",
*_DATES,
"closed_on__gte",
"closed_on__lte",
"amount__gte",
"amount__lte",
"open",
"rotten",
}
),
),
"cases": (
lambda request, params: case_list_queryset(request.profile, params),
frozenset(
{
"include_deleted",
"show_merged",
"name",
"status",
"priority",
"account",
"case_type",
"assigned_to",
"tags",
"search",
*_DATES,
"sla_breached",
"ordering",
}
),
),
"invoices": (
lambda request, params: filter_invoices(
visible_invoices_qs(request.profile, request.user), params
),
frozenset(
{
"search",
"status",
"account",
"contact",
"opportunity",
"assigned_to",
"created_by",
"issue_date_gte",
"issue_date_lte",
"due_date_gte",
"due_date_lte",
"sort",
}
),
),
}
def _clean_filters(module, value):
"""``value`` as ``{param: [text, ...]}``, or raise with what is wrong.
A value may arrive as one string or a list of them; blanks are dropped, and
a parameter left with nothing is dropped with them.
"""
if not isinstance(value, dict):
raise serializers.ValidationError("Send the filters as an object.")
if len(value) > MAX_PARAMS:
raise serializers.ValidationError(
f"A view can hold at most {MAX_PARAMS} filters."
)
accepted = LISTS[module][1]
unknown = sorted(
str(key)[:50]
for key in value
if not isinstance(key, str)
or (key not in accepted and not CUSTOM_FIELD_PARAM.match(key))
)
if unknown:
raise serializers.ValidationError(
f"This list does not filter by: {', '.join(unknown[:5])}."
)
cleaned = {}
for key, raw in value.items():
values = raw if isinstance(raw, list) else [raw]
if not all(isinstance(v, str) for v in values):
raise serializers.ValidationError(f"{key}: every value must be text.")
values = [v.strip() for v in values if v.strip()]
if len(values) > MAX_VALUES_PER_PARAM:
raise serializers.ValidationError(
f"{key}: at most {MAX_VALUES_PER_PARAM} values."
)
if any(len(v) > MAX_VALUE_LENGTH for v in values):
raise serializers.ValidationError(
f"{key}: a value can be at most {MAX_VALUE_LENGTH} characters."
)
if values:
cleaned[key] = values
return cleaned
def _check_values(request, module, filters):
"""Run the list's own parameter parsing over ``filters``.
The queryset is built, never evaluated. A malformed id, date or number
raises the list's own ``ValidationError`` while it is being built.
"""
params = QueryDict(mutable=True)
for key, values in filters.items():
params.setlist(key, values)
try:
LISTS[module][0](request, params)
except serializers.ValidationError as exc:
# The list names the parameter it refused; kept as one flat sentence
# per parameter, which is what both clients show.
detail = exc.detail
if isinstance(detail, dict):
detail = [
f"{key}: {value[0] if isinstance(value, list) else value}"
for key, value in detail.items()
]
raise serializers.ValidationError({"filters": detail}) from None
class SavedViewSerializer(serializers.ModelSerializer):
"""One saved view, as its owner reads and writes it.
``org`` and ``profile`` are set by the view from the request, and a body
naming either is refused rather than ignored. ``module`` is fixed once the
view exists: a view's filters only mean something to the list they came
from.
"""
class Meta:
model = SavedView
fields = ("id", "module", "name", "filters", "created_at", "updated_at")
read_only_fields = ("id", "created_at", "updated_at")
def validate_name(self, value):
value = value.strip()
if not value:
raise serializers.ValidationError("Give the view a name.")
return value
def validate(self, attrs):
for field in ("org", "profile"):
if field in self.initial_data:
raise serializers.ValidationError({field: "This is set by the server."})
instance = self.instance
if instance is not None and attrs.get("module", instance.module) != (
instance.module
):
raise serializers.ValidationError(
{"module": "A saved view cannot move to another list."}
)
module = attrs.get("module") or instance.module
if "filters" in attrs:
try:
attrs["filters"] = _clean_filters(module, attrs["filters"])
except serializers.ValidationError as exc:
raise serializers.ValidationError({"filters": exc.detail}) from None
_check_values(self.context["request"], module, attrs["filters"])
profile = self.context["request"].profile
mine = SavedView.objects.filter(org=profile.org, profile=profile, module=module)
if instance is not None:
mine = mine.exclude(pk=instance.pk)
elif mine.count() >= MAX_VIEWS_PER_LIST:
raise serializers.ValidationError(
f"You can save at most {MAX_VIEWS_PER_LIST} views for one list."
)
name = attrs.get("name")
if name is not None and mine.filter(name__iexact=name).exists():
raise serializers.ValidationError(
{"name": "You already have a view with this name for this list."}
)
return attrs
def save(self, **kwargs):
# Two saves racing past the check above meet the unique constraint;
# answered as the same 400, not a 500.
try:
with transaction.atomic():
return super().save(**kwargs)
except IntegrityError:
raise serializers.ValidationError(
{"name": "You already have a view with this name for this list."}
) from None

View file

@ -83,6 +83,8 @@ API_RESOURCES = frozenset(
"portal",
"profile",
"public",
# A profile's own saved list filters (G29).
"saved-views",
"schema",
"search",
"tags",
@ -126,6 +128,10 @@ CREDENTIAL_PATHS = (
# is revoked. Same persistence argument as minting a token, so webhooks are
# managed from a signed-in session only.
"/api/webhooks/",
# The security audit log records who signed in from where and what was
# refused. Read from a signed-in admin session only, like webhooks: a
# leaked token should not also reveal what the org has noticed about it.
"/api/org/audit-log/",
)
# What the organization API key is worth once this module is enforcing. It reads,

View file

@ -49,7 +49,13 @@ class OrgAwareRefreshToken(RefreshToken):
- org_id: Organization UUID
- org_name: Organization name (for display)
- role: User's role in the org (ADMIN/USER)
- is_organization_admin: ``is_org_admin(profile)``, the one admin fact
both clients gate admin UI on (the role, or a superuser's profile)
- org_settings: Currency and locale settings
The clients read these claims for display and to hide controls only. The
API never trusts them: every request re-resolves the profile from the
database, and ``OrgAwareTokenRefreshView`` re-derives them on each refresh.
"""
@classmethod
@ -94,6 +100,7 @@ class OrgAwareRefreshToken(RefreshToken):
# Add role if profile provided (avoids /api/auth/profile call)
if profile:
token["role"] = profile.role
token["is_organization_admin"] = is_org_admin(profile)
return token
@ -584,6 +591,13 @@ class ShowOrganizationListSerializer(serializers.ModelSerializer):
"""
org = OrganizationSerializer()
# Computed, not the column: the column mirrors `role` only, and this is
# the fact the clients gate admin UI on. Read-only by construction.
is_organization_admin = serializers.SerializerMethodField()
@extend_schema_field(bool)
def get_is_organization_admin(self, obj):
return is_org_admin(obj)
class Meta:
model = Profile
@ -626,6 +640,10 @@ class CreateUserSerializer(serializers.ModelSerializer):
def __init__(self, *args, **kwargs):
self.org = kwargs.pop("org", None)
# Whether the caller is editing their own account. Defaults to False so
# a caller that forgets to pass it fails closed: the account fields
# below are then refused rather than written.
self._editing_self = kwargs.pop("editing_self", False)
super().__init__(*args, **kwargs)
self.fields["email"].required = True
# Membership is per-org: one account can belong to several orgs, so the
@ -641,25 +659,39 @@ class CreateUserSerializer(serializers.ModelSerializer):
def validate_email(self, email):
if self.instance:
# The login email is read-only through this API, for an admin
# editing someone else and for the user themselves (owner decision,
# 1.11.0). `User.email` is global: an org admin who could rename a
# member's email could then sign in as that person in every org
# they belong to, and as a platform superadmin if they are one.
# Echoing the stored address back (any case) is not a change, so
# forms that resend the whole record keep working; the stored
# spelling is kept rather than the echoed one.
if self.instance.email.lower() == email.lower():
return email
# Renaming an account: the address must be free globally, since
# there is no way to fold two existing accounts together.
if (
User.objects.filter(email__iexact=email)
.exclude(pk=self.instance.pk)
.exists()
):
raise serializers.ValidationError("Email already exists")
if Profile.objects.filter(user__email__iexact=email, org=self.org).exists():
raise serializers.ValidationError("Email already exists")
return email
return self.instance.email
raise serializers.ValidationError("Email cannot be changed here.")
# Creating a membership: an account owned elsewhere is reused by the
# view, so only same-org duplicates are rejected here.
if not Profile.objects.filter(user__email__iexact=email, org=self.org).exists():
return email.lower()
raise serializers.ValidationError("Given Email id already exists")
def validate_profile_pic(self, value):
"""Only the account's owner may change it once the account exists.
`User` is one row shared by every org the person belongs to, so an org
admin editing a member here would be rewriting that person's account
for their other orgs too (owner decision, 1.11.0). An admin resending
the stored value, as a whole-record form does, is not a change.
"""
if self.instance is None or self._editing_self:
return value
if (value or "") == (self.instance.profile_pic or ""):
return self.instance.profile_pic
raise serializers.ValidationError(
"Only the account's owner can change their picture."
)
class CreateProfileSerializer(serializers.ModelSerializer):
class Meta:
@ -1024,15 +1056,33 @@ class APISettingsListSerializer(serializers.ModelSerializer):
return data
class APISettingsSwaggerSerializer(serializers.ModelSerializer):
class Meta:
model = APISettings
fields = [
"title",
"website",
"lead_assigned_to",
"tags",
]
class APISettingsSwaggerSerializer(serializers.Serializer):
"""The request body `common/views/settings_views.py` actually reads.
A ModelSerializer here documented both relations as lists of ids, while the
views have always taken `tags` as names. Written out by hand so the schema
says what the endpoint does. On PATCH every field is optional, and an
absent `tags` or `lead_assigned_to` is left unchanged.
"""
title = serializers.CharField()
website = serializers.URLField(max_length=255)
tags = serializers.ListField(
child=serializers.CharField(),
required=False,
help_text=(
"Tag NAMES, not ids. Each is matched by slug within the caller's "
"org and created there if the org has none by that name."
),
)
lead_assigned_to = serializers.ListField(
child=serializers.UUIDField(),
required=False,
help_text=(
"Profile ids. Each must be an active member of the caller's org, "
"or the request is refused with a 400."
),
)
class DocumentCreateSwaggerSerializer(serializers.ModelSerializer):
@ -1128,13 +1178,15 @@ class UserDetailSerializer(serializers.ModelSerializer):
@extend_schema_field(list)
def get_organizations(self, obj):
"""Get all organizations the user belongs to"""
profiles = Profile.objects.filter(user=obj, is_active=True)
profiles = Profile.objects.filter(user=obj, is_active=True).select_related(
"org", "user"
)
return [
{
"id": str(profile.org.id),
"name": profile.org.name,
"role": profile.role,
"is_organization_admin": profile.is_organization_admin,
"is_organization_admin": is_org_admin(profile),
"has_sales_access": profile.has_sales_access,
"has_marketing_access": profile.has_marketing_access,
}
@ -1162,11 +1214,17 @@ class ProfileDetailSerializer(serializers.ModelSerializer):
org = OrganizationSerializer(read_only=True)
teams = serializers.SerializerMethodField()
last_login = serializers.DateTimeField(source="user.last_login", read_only=True)
# `is_org_admin`, not the column; see `ShowOrganizationListSerializer`.
is_organization_admin = serializers.SerializerMethodField()
@extend_schema_field(serializers.ListField(child=serializers.CharField()))
def get_teams(self, obj):
return list(obj.user_teams.values_list("name", flat=True))
@extend_schema_field(bool)
def get_is_organization_admin(self, obj):
return is_org_admin(obj)
class Meta:
model = Profile
fields = [

View file

@ -7,8 +7,10 @@ with no org, which cannot be saved, so that branch always answered 500.
``lead_assigned_to`` was ``.add(*ids)`` straight from the body: another org's
profile id attached, and ``webforms/legacy.py`` then hands leads to it.
The request contract is unchanged: ``tags`` is a list of tag names and
``lead_assigned_to`` a list of profile ids.
The request contract: ``tags`` is a list of tag names and ``lead_assigned_to``
a list of profile ids, on POST, PUT and PATCH alike (Swagger used to say ids
for both, and PATCH used to ignore both). An assignee that is not an active
member of the caller's org is a 400, not silently dropped.
"""
import pytest
@ -123,24 +125,43 @@ class TestLeadAssignedTo:
assert response.status_code in (200, 201), response.data
assert list(setting.lead_assigned_to.all()) == [admin_profile]
def test_another_orgs_profile_is_not_attached(
self, write, admin_profile, profile_b
):
def test_another_orgs_profile_is_refused(self, write, admin_profile, profile_b):
response, setting = write(
_body(lead_assigned_to=[str(admin_profile.id), str(profile_b.id)])
_body(
title="Foreign",
lead_assigned_to=[str(admin_profile.id), str(profile_b.id)],
)
)
assert response.status_code in (200, 201), response.data
assert list(setting.lead_assigned_to.all()) == [admin_profile]
assert response.status_code == status.HTTP_400_BAD_REQUEST
assert "lead_assigned_to" in response.data["errors"]
if write.method == "post":
assert setting is None
else:
assert setting.title == "Existing"
assert setting.lead_assigned_to.count() == 0
def test_inactive_profile_is_not_attached(self, write, user_profile):
def test_inactive_profile_is_refused(self, write, user_profile):
user_profile.is_active = False
user_profile.save()
response, setting = write(_body(lead_assigned_to=[str(user_profile.id)]))
response, setting = write(
_body(title="Inactive", lead_assigned_to=[str(user_profile.id)])
)
assert response.status_code in (200, 201), response.data
assert setting.lead_assigned_to.count() == 0
assert response.status_code == status.HTTP_400_BAD_REQUEST
assert "lead_assigned_to" in response.data["errors"]
def test_unknown_id_is_refused(self, write):
response, _setting_obj = write(
_body(
title="Unknown",
lead_assigned_to=["00000000-0000-0000-0000-000000000000"],
)
)
assert response.status_code == status.HTTP_400_BAD_REQUEST
assert "lead_assigned_to" in response.data["errors"]
@pytest.mark.parametrize(
"assignees", [["not-a-uuid"], "[not json", {"id": 1}, 5, [5]]
@ -170,3 +191,98 @@ def test_put_on_another_orgs_setting_is_404(admin_client, org_b, user_b):
assert response.status_code == status.HTTP_404_NOT_FOUND
setting.refresh_from_db()
assert setting.title == "Existing"
@pytest.mark.django_db
class TestPatch:
"""PATCH used to save `title`/`website` and drop both relations unread."""
def _patch(self, client, setting, body):
response = client.patch(_detail_url(setting.pk), body, format="json")
setting.refresh_from_db()
return response
def test_tags_by_name_replace_inside_the_callers_org(
self, admin_client, org_a, org_b, admin_user
):
setting = _setting(org_a, admin_user)
old = Tags.objects.create(name="Old", org=org_a)
setting.tags.add(old)
foreign = Tags.objects.create(name="VIP", org=org_b)
response = self._patch(admin_client, setting, {"tags": ["VIP"]})
assert response.status_code == status.HTTP_200_OK, response.data
attached = list(setting.tags.all())
assert len(attached) == 1
assert attached[0].org_id == org_a.id
assert attached[0].pk != foreign.pk
def test_malformed_tags_are_400_and_change_nothing(
self, admin_client, org_a, admin_user
):
setting = _setting(org_a, admin_user)
response = self._patch(admin_client, setting, {"title": "New", "tags": [5]})
assert response.status_code == status.HTTP_400_BAD_REQUEST
assert "tags" in response.data["errors"]
assert setting.title == "Existing"
def test_assignees_replace_with_an_active_org_member(
self, admin_client, org_a, admin_user, admin_profile, user_profile
):
setting = _setting(org_a, admin_user)
setting.lead_assigned_to.add(admin_profile)
response = self._patch(
admin_client, setting, {"lead_assigned_to": [str(user_profile.id)]}
)
assert response.status_code == status.HTTP_200_OK, response.data
assert list(setting.lead_assigned_to.all()) == [user_profile]
def test_another_orgs_profile_is_refused(
self, admin_client, org_a, admin_user, admin_profile, profile_b
):
setting = _setting(org_a, admin_user)
setting.lead_assigned_to.add(admin_profile)
response = self._patch(
admin_client, setting, {"lead_assigned_to": [str(profile_b.id)]}
)
assert response.status_code == status.HTTP_400_BAD_REQUEST
assert "lead_assigned_to" in response.data["errors"]
assert list(setting.lead_assigned_to.all()) == [admin_profile]
def test_absent_relations_are_left_alone(
self, admin_client, org_a, admin_user, admin_profile
):
setting = _setting(org_a, admin_user)
tag = Tags.objects.create(name="Keep", org=org_a)
setting.tags.add(tag)
setting.lead_assigned_to.add(admin_profile)
response = self._patch(admin_client, setting, {"title": "Renamed"})
assert response.status_code == status.HTTP_200_OK, response.data
assert setting.title == "Renamed"
assert list(setting.tags.all()) == [tag]
assert list(setting.lead_assigned_to.all()) == [admin_profile]
def test_member_is_refused(self, user_client, org_a, admin_user):
setting = _setting(org_a, admin_user)
response = self._patch(user_client, setting, {"tags": ["VIP"]})
assert response.status_code == status.HTTP_403_FORBIDDEN
assert setting.tags.count() == 0
def test_swagger_documents_tags_as_names_and_assignees_as_ids():
from common.serializer import APISettingsSwaggerSerializer
fields = APISettingsSwaggerSerializer().fields
assert fields["tags"].child.__class__.__name__ == "CharField"
assert fields["lead_assigned_to"].child.__class__.__name__ == "UUIDField"

View file

@ -28,14 +28,16 @@ class TestAuditLoggerMethods:
self.factory = RequestFactory()
def test_get_request_info_with_forwarded_for(self):
"""X-Forwarded-For should be parsed for the first IP."""
"""The first X-Forwarded-For entry is the caller's to write, so it is
not recorded. With no proxy configured the socket peer is."""
request = self.factory.get(
"/api/test/",
HTTP_X_FORWARDED_FOR="203.0.113.50, 70.41.3.18, 150.172.238.178",
HTTP_USER_AGENT="TestAgent/1.0",
REMOTE_ADDR="192.0.2.10",
)
info = self.logger._get_request_info(request)
assert info["ip_address"] == "203.0.113.50"
assert info["ip_address"] == "192.0.2.10"
assert info["user_agent"] == "TestAgent/1.0"
assert info["request_path"] == "/api/test/"
assert info["request_method"] == "GET"

View file

@ -0,0 +1,231 @@
"""GET /api/org/audit-log/: an admin reads their own org's security audit log.
`security_audit_log` has no RLS policy, so the view's explicit org filter is
the only thing keeping one org's rows from another. These tests plant rows in
two orgs and with no org at all, and check only the caller's come back.
"""
from datetime import timedelta
import pytest
from django.test import Client
from django.utils import timezone
from common.audit_log import SecurityAuditLog
from common.models import PersonalAccessToken, Profile, User
from common.testing import _make_authenticated_client
URL = "/api/org/audit-log/"
def _row(org, event_type="LOGIN_SUCCESS", user=None, days_ago=0, **extra):
row = SecurityAuditLog.objects.create(
org=org, event_type=event_type, user=user, **extra
)
if days_ago:
SecurityAuditLog.objects.filter(pk=row.pk).update(
created_at=timezone.now() - timedelta(days=days_ago)
)
return row
def _ids(response):
return [entry["id"] for entry in response.json()["results"]]
class TestWhoMayRead:
def test_an_admin_reads_their_own_org_only(
self, admin_client, admin_user, org_a, org_b
):
ours = _row(org_a, user=admin_user)
_row(org_b, user=admin_user)
_row(None, event_type="LOGIN_FAILURE", success=False)
response = admin_client.get(URL)
assert response.status_code == 200
assert _ids(response) == [str(ours.id)]
assert response.json()["count"] == 1
def test_another_orgs_admin_never_sees_them(self, org_b_client, org_a, org_b):
_row(org_a)
theirs = _row(org_b)
assert _ids(org_b_client.get(URL)) == [str(theirs.id)]
def test_a_member_is_refused(self, user_client, org_a):
_row(org_a)
assert user_client.get(URL).status_code == 403
def test_a_superuser_holding_the_user_role_is_an_admin(self, org_a):
user = User.objects.create_user(
email="root@test.com", password="x", is_superuser=True
)
profile = Profile.objects.create(user=user, org=org_a, role="USER")
client = _make_authenticated_client(user, org_a, profile)
assert client.get(URL).status_code == 200
def test_an_admins_api_token_is_refused(self, admin_profile, org_a):
_row(org_a)
raw, _ = PersonalAccessToken.generate(profile=admin_profile, name="cli")
auth = {"HTTP_AUTHORIZATION": f"Bearer {raw}"}
client = Client()
assert client.get(URL, **auth).status_code == 403
# The same token still reaches an ordinary endpoint.
assert client.get("/api/leads/", **auth).status_code == 200
def test_the_org_api_key_is_refused(self, admin_profile, org_a):
_row(org_a)
client = Client()
assert client.get(URL, HTTP_TOKEN=org_a.api_key).status_code == 403
# The key itself works: it still reads an ordinary endpoint.
assert client.get("/api/leads/", HTTP_TOKEN=org_a.api_key).status_code == 200
def test_anonymous_is_refused(self, unauthenticated_client):
assert unauthenticated_client.get(URL).status_code in (401, 403)
class TestFilters:
def test_newest_first_and_paginated(self, admin_client, org_a):
old = _row(org_a, days_ago=3)
new = _row(org_a)
body = admin_client.get(f"{URL}?limit=1").json()
assert body["count"] == 2
assert [e["id"] for e in body["results"]] == [str(new.id)]
body = admin_client.get(f"{URL}?limit=1&offset=1").json()
assert [e["id"] for e in body["results"]] == [str(old.id)]
def test_limit_is_capped(self, admin_client, org_a):
for _ in range(3):
_row(org_a)
assert len(admin_client.get(f"{URL}?limit=100000").json()["results"]) == 3
def test_by_event_type(self, admin_client, org_a):
_row(org_a, event_type="LOGIN_SUCCESS")
switch = _row(org_a, event_type="ORG_SWITCH")
assert _ids(admin_client.get(f"{URL}?event_type=ORG_SWITCH")) == [
str(switch.id)
]
def test_by_actor(self, admin_client, admin_user, regular_user, org_a):
_row(org_a, user=admin_user)
theirs = _row(org_a, user=regular_user)
assert _ids(admin_client.get(f"{URL}?actor={regular_user.id}")) == [
str(theirs.id)
]
def test_by_date_range(self, admin_client, org_a):
_row(org_a, days_ago=10)
middle = _row(org_a, days_ago=5)
_row(org_a)
today = timezone.localdate()
start = (today - timedelta(days=6)).isoformat()
end = (today - timedelta(days=4)).isoformat()
assert _ids(admin_client.get(f"{URL}?from={start}&to={end}")) == [
str(middle.id)
]
@pytest.mark.parametrize(
"query",
[
"event_type=NOT_A_TYPE",
"actor=not-a-uuid",
"from=yesterday",
"to=2026-02-30",
"from=2026-09-10&to=2026-09-01",
],
)
def test_a_malformed_filter_is_400(self, admin_client, org_a, query):
assert admin_client.get(f"{URL}?{query}").status_code == 400
def test_the_filter_catalogue_comes_with_the_page(self, admin_client):
types = admin_client.get(URL).json()["event_types"]
assert {"value": "WEBHOOK_PAUSED", "label": "Webhook Paused"} in types
class TestWhatIsShown:
def test_an_entry_carries_only_allow_listed_fields(
self, admin_client, admin_user, org_a
):
_row(
org_a,
event_type="SUSPICIOUS_ACTIVITY",
user=admin_user,
description="Switched from Other Tenant Ltd to here",
metadata={
"details": "Bearer bcrm_pat_secret",
"api_key_prefix": "abcd",
"email": "someone@else.com",
"action": "export",
"endpoint_id": "e1",
},
ip_address="203.0.113.9",
request_method="GET",
request_path="/api/leads/",
success=False,
)
entry = admin_client.get(URL).json()["results"][0]
assert set(entry) == {
"id",
"event_type",
"event_label",
"created_at",
"success",
"actor",
"ip_address",
"user_agent",
"request_method",
"request_path",
"details",
}
assert entry["details"] == {"action": "export", "endpoint_id": "e1"}
assert entry["actor"]["email"] == admin_user.email
assert entry["event_label"] == "Suspicious Activity"
assert "Other Tenant" not in str(entry)
assert "bcrm_pat_secret" not in str(entry)
def test_a_public_path_is_cut_to_its_prefix(self, admin_client, org_a):
_row(org_a, request_path="/api/public/csat/tok_abc123/")
entry = admin_client.get(URL).json()["results"][0]
assert entry["request_path"] == "/api/public/"
class TestTheRecordedIp:
"""The viewer shows `ip_address` as fact, so the caller cannot choose it.
A client can send any `X-Forwarded-For`; only the entries the proxies we
run appended (`REST_FRAMEWORK["NUM_PROXIES"]`, counted from the right) are
believed, and with no proxy configured the header is ignored.
"""
def _logged_ip(self, org, **meta):
from django.test import RequestFactory
from common.audit_log import audit_log
request = RequestFactory().get("/api/auth/logout/", **meta)
audit_log.logout(None, org, request)
return (
SecurityAuditLog.objects.filter(event_type="LOGOUT")
.latest("created_at")
.ip_address
)
def test_a_forged_header_is_ignored_with_no_proxy_configured(self, org_a):
ip = self._logged_ip(
org_a, HTTP_X_FORWARDED_FOR="6.6.6.6", REMOTE_ADDR="198.51.100.7"
)
assert ip == "198.51.100.7"
def test_behind_one_proxy_only_its_entry_is_believed(self, org_a, settings):
settings.REST_FRAMEWORK = {**settings.REST_FRAMEWORK, "NUM_PROXIES": 1}
ip = self._logged_ip(
org_a,
HTTP_X_FORWARDED_FOR="6.6.6.6, 203.0.113.9",
REMOTE_ADDR="127.0.0.1",
)
assert ip == "203.0.113.9"
def test_a_junk_entry_records_nothing_rather_than_junk(self, org_a, settings):
settings.REST_FRAMEWORK = {**settings.REST_FRAMEWORK, "NUM_PROXIES": 1}
ip = self._logged_ip(
org_a, HTTP_X_FORWARDED_FOR="not-an-ip", REMOTE_ADDR="127.0.0.1"
)
assert ip is None

View file

@ -5,12 +5,12 @@
the failed logins, invalid API keys and cross-org attempts the ledger exists
to record. Full reasoning is in that migration.
Removing a safety net is only safe while nothing depends on it. Today
`SecurityAuditLog` is write-only: `AuditLogger` creates rows, `common/models.py`
re-exports the name, and one pack test reads it. The moment an endpoint serves
these rows, a missing `org=` filter becomes a cross-tenant disclosure of
Removing a safety net means every reader has to filter for itself. One
endpoint serves these rows, `common/views/audit_log_views.py` (G33), and it
filters on the caller's org explicitly; `test_audit_log_api.py` pins that.
Anywhere else, a missing `org=` filter would be a cross-tenant disclosure of
usernames, IP addresses and authentication failures with no policy underneath
to catch it. These tests fail when that day arrives, next to the reason.
to catch it, so a new reference fails here, next to the reason.
"""
import re
@ -30,6 +30,10 @@ ALLOWED = {
"common/models.py", # re-exports the name for existing imports
"common/migrations", # schema history
"common/tests", # this guard, plus one pack test asserting a row was written
"webhooks/tests", # asserts the pause and re-enable rows are written
# Serves rows to admins. Filters on `org=request.profile.org` explicitly and
# returns an allow-list of fields; pinned by common/tests/test_audit_log_api.py.
"common/views/audit_log_views.py",
}

View file

@ -121,8 +121,13 @@ class TestOrdinaryEditStillWorks:
comment_on_task.refresh_from_db()
assert comment_on_task.comment == "Fixed my typo"
def test_a_third_party_still_cannot_edit(self, user_client, comment_on_task):
"""Author-or-admin gating is untouched by the field change."""
def test_a_third_party_still_cannot_edit(
self, user_client, user_profile, task_a, comment_on_task
):
"""Author-or-admin gating is untouched by the field change. The third
party can read the task: one who cannot gets the 404 a missing comment
gets (`tasks/tests/test_attachment_and_comment_reach_only_visible_tasks.py`)."""
task_a.assigned_to.add(user_profile)
response = user_client.put(
f"/api/tasks/comment/{comment_on_task.id}/",
{"comment": "Not mine to edit"},

View file

@ -0,0 +1,52 @@
"""`scripts/generate_endpoint_index.py` keeps the deprecation notes.
The index is regenerated whenever routes change, so a note typed into the
generated file is lost on the next run. The deprecation is read from the
schema instead, and the script's own mapping only says what to use in its
place. This renders rows without writing the file.
"""
import importlib.util
from pathlib import Path
import pytest
from django.conf import settings
from drf_spectacular.generators import SchemaGenerator
SCRIPT = Path(settings.BASE_DIR) / "scripts" / "generate_endpoint_index.py"
@pytest.fixture(scope="module")
def generator():
spec = importlib.util.spec_from_file_location("generate_endpoint_index", SCRIPT)
module = importlib.util.module_from_spec(spec)
spec.loader.exec_module(module)
return module
@pytest.fixture(scope="module")
def rows(generator):
return generator.render(SchemaGenerator().get_schema(request=None, public=True))
def test_the_lead_upload_note_survives_regeneration(rows):
assert (
"| `/api/leads/upload/` | POST (deprecated; use `import/preview/` + "
"`import/commit/`) |" in rows
)
def test_every_deprecated_operation_is_marked(rows):
row = next(r for r in rows if r.startswith("| `/api/leads/create-from-site/`"))
assert "POST (deprecated; use `/api/public/forms/" in row
def test_a_live_operation_is_not_marked(rows):
assert "| `/api/leads/export/` | GET |" in rows
assert "| `/api/saved-views/` | GET, POST |" in rows
def test_a_note_on_an_operation_the_code_no_longer_deprecates_is_refused(generator):
schema = {"paths": {"/api/leads/upload/": {"post": {"deprecated": False}}}}
with pytest.raises(SystemExit, match="POST /api/leads/upload/"):
generator.render(schema)

View file

@ -11,6 +11,8 @@ Every check asserts BOTH directions. A permission test that only ever sees
one answer cannot tell a working check from a constant.
"""
import uuid
import pytest
from django.core.files.uploadedfile import SimpleUploadedFile
@ -18,7 +20,10 @@ from accounts.models import Account
from cases.models import Case, CaseWatcher
from common.models import Attachments, Document, Teams
from common.utils import create_attachment
from contacts.models import Contact
from invoices.models import Invoice
from leads.models import Lead
from opportunity.models import Opportunity
from tasks.models import Task
FILE_BYTES = b"the quick brown fox"
@ -149,8 +154,9 @@ class TestAttachmentDownload:
lead = Lead.objects.create(title="Not mine", org=org_a)
Lead.objects.filter(pk=lead.pk).update(created_by=admin_user)
attachment = _attach(lead, admin_profile)
assert user_client.get(f"/api/leads/{lead.pk}/").status_code == 403
assert user_client.get(_attachment_url(attachment.pk)).status_code == 403
# The lead answers as if missing, and so does its file.
assert user_client.get(f"/api/leads/{lead.pk}/").status_code == 404
assert user_client.get(_attachment_url(attachment.pk)).status_code == 404
def test_assignment_to_the_lead_is_enough(
self, user_client, admin_user, admin_profile, user_profile, org_a
@ -168,7 +174,7 @@ class TestAttachmentDownload:
case = Case.objects.create(name="Watched", org=org_a, status="New")
Case.objects.filter(pk=case.pk).update(created_by=admin_user)
attachment = _attach(case, admin_profile)
assert user_client.get(_attachment_url(attachment.pk)).status_code == 403
assert user_client.get(_attachment_url(attachment.pk)).status_code == 404
# `watchers` goes through CaseWatcher, which carries its own org FK.
CaseWatcher.objects.create(case=case, profile=user_profile, org=org_a)
assert user_client.get(_attachment_url(attachment.pk)).status_code == 200
@ -181,7 +187,7 @@ class TestAttachmentDownload:
)
Task.objects.filter(pk=task.pk).update(created_by=admin_user)
attachment = _attach(task, admin_profile)
assert user_client.get(_attachment_url(attachment.pk)).status_code == 403
assert user_client.get(_attachment_url(attachment.pk)).status_code == 404
task.assigned_to.add(user_profile)
assert user_client.get(_attachment_url(attachment.pk)).status_code == 200
@ -222,7 +228,7 @@ class TestAttachmentDownload:
Lead.objects.filter(pk=lead.pk).update(created_by=admin_user)
attachment = _attach(lead, admin_profile)
Lead.objects.filter(pk=lead.pk).delete()
assert admin_client.get(_attachment_url(attachment.pk)).status_code == 403
assert admin_client.get(_attachment_url(attachment.pk)).status_code == 404
def test_an_unmapped_content_type_is_refused(
self, admin_client, admin_profile, org_a
@ -235,7 +241,7 @@ class TestAttachmentDownload:
team = Teams.objects.create(name="Support", org=org_a)
attachment = _attach(team, admin_profile)
assert attachment.content_type.model == "teams"
assert admin_client.get(_attachment_url(attachment.pk)).status_code == 403
assert admin_client.get(_attachment_url(attachment.pk)).status_code == 404
def test_the_download_is_named_after_the_stored_file_name(
self, admin_client, admin_user, admin_profile, org_a
@ -254,3 +260,61 @@ class TestAttachmentDownload:
attachment = _attach(lead, admin_profile)
Attachments.objects.filter(pk=attachment.pk).update(attachment="")
assert admin_client.get(_attachment_url(attachment.pk)).status_code == 404
def _record(kind, org):
"""One record of each attachable kind, owned by nobody in particular."""
if kind == "lead":
return Lead.objects.create(title="Hidden", org=org)
if kind == "opportunity":
return Opportunity.objects.create(name="Hidden", stage="QUALIFICATION", org=org)
if kind == "contact":
return Contact.objects.create(first_name="Hidden", last_name="Person", org=org)
if kind == "account":
return Account.objects.create(name="Hidden", org=org)
if kind == "case":
return Case.objects.create(name="Hidden", org=org, status="New")
if kind == "task":
return Task.objects.create(
title="Hidden", org=org, status="New", priority="Low"
)
account = Account.objects.create(name="Billed", org=org)
return Invoice.objects.create(org=org, account=account, invoice_title="Hidden")
KINDS = ["lead", "opportunity", "contact", "account", "case", "task", "invoice"]
@pytest.mark.django_db
class TestHiddenFileAnswersLikeAMissingOne:
"""A file on a record the caller cannot open is indistinguishable from an id
that does not exist: same status, same body, on every module's records."""
@pytest.mark.parametrize("kind", KINDS)
def test_whole_response_matches_a_missing_id(
self, kind, user_client, admin_client, admin_user, admin_profile, org_a
):
record = _record(kind, org_a)
type(record).objects.filter(pk=record.pk).update(created_by=admin_user)
attachment = _attach(record, admin_profile)
hidden = user_client.get(_attachment_url(attachment.pk))
missing = user_client.get(_attachment_url(uuid.uuid4()))
assert hidden.status_code == missing.status_code == 404
assert hidden.content == missing.content
assert hidden["Content-Type"] == missing["Content-Type"]
# The check refuses the member, not everyone: the admin gets the bytes.
allowed = admin_client.get(_attachment_url(attachment.pk))
assert allowed.status_code == 200
assert _body(allowed) == FILE_BYTES
def test_another_orgs_file_matches_a_missing_id(
self, org_b_client, admin_user, admin_profile, org_a
):
record = _record("case", org_a)
attachment = _attach(record, admin_profile)
other = org_b_client.get(_attachment_url(attachment.pk))
missing = org_b_client.get(_attachment_url(uuid.uuid4()))
assert other.status_code == missing.status_code == 404
assert other.content == missing.content

View file

@ -17,9 +17,9 @@ search or dashboard hid, or be shown one that answered 403:
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.
Since 1.11.0 a Django superuser is an org admin everywhere (``is_org_admin``
admits them), so tickets and tasks admit one too. Each test here pins one path
against the detail answer for the same caller.
"""
import pytest
@ -138,7 +138,7 @@ 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 response.status_code == status.HTTP_404_NOT_FOUND
assert str(others_account.id) not in _account_ids(path, user_client)
@pytest.mark.parametrize("path", ACCOUNT_PATHS)
@ -214,11 +214,18 @@ class TestDashboardAccountCount:
class TestDashboardTasks:
"""Tasks have no superuser clause, so the dashboard must not invent one."""
"""The dashboard counts exactly the tasks the detail view opens."""
def test_superuser_does_not_get_a_task_they_cannot_open(
def test_superuser_is_an_admin_so_list_and_detail_both_admit_the_task(
self, superuser, user_client, others_task
):
response = user_client.get(f"/api/tasks/{others_task.pk}/")
assert response.status_code == status.HTTP_200_OK
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_plain_member_gets_neither(self, 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)
@ -265,16 +272,14 @@ class TestTodayFollowsTheDetailRule:
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(
def test_superuser_is_an_admin_so_opens_the_rows_it_is_shown(
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
assert f"case-{others_case.id}" in ids
assert f"task-{others_task.id}" in ids
for url in (f"/api/cases/{others_case.pk}/", f"/api/tasks/{others_task.pk}/"):
assert user_client.get(url).status_code == status.HTTP_200_OK
def test_another_orgs_rows_never_appear(
self, user_profile, user_client, others_case, foreign_case, foreign_task
@ -311,10 +316,10 @@ class TestCaseBoard:
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(
def test_superuser_is_an_admin_and_sees_it(
self, superuser, user_client, others_case
):
assert str(others_case.id) not in _case_board(user_client)
assert str(others_case.id) in _case_board(user_client)
def test_another_orgs_ticket_never_appears(
self, admin_client, others_case, foreign_case
@ -341,10 +346,10 @@ class TestTaskBoard:
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(
def test_superuser_is_an_admin_and_sees_it(
self, superuser, user_client, others_task
):
assert str(others_task.id) not in _task_board(user_client)
assert str(others_task.id) in _task_board(user_client)
def test_another_orgs_task_never_appears(
self, admin_client, others_task, foreign_task

View file

@ -0,0 +1,205 @@
"""The login email cannot be changed through the user API (owner decision, 1.11.0).
``User.email`` is the account's global sign-in identity. ``PUT`` and ``PATCH
/api/user/<id>/`` let an org admin rename another member's email, answered 200:
the admin could then take a magic link to the new address and sign in as that
person in every org they belong to, and as platform superadmin if they were
one. Now a changed email is a 400 for everyone, self included. Echoing the
stored address (any case) is accepted so forms that resend the record work.
"""
import pytest
from rest_framework import status
def _url(user):
return f"/api/user/{user.id}/"
VERBS = ["put", "patch"]
@pytest.mark.django_db
@pytest.mark.parametrize("verb", VERBS)
def test_admin_cannot_change_another_members_email(
verb, admin_client, regular_user, user_profile
):
response = getattr(admin_client, verb)(
_url(regular_user),
{"email": "attacker@evil.test", "role": "USER", "phone": "+14155550100"},
format="json",
)
assert response.status_code == status.HTTP_400_BAD_REQUEST
assert "Email cannot be changed here." in str(response.data)
regular_user.refresh_from_db()
user_profile.refresh_from_db()
assert regular_user.email == "user@test.com"
# Refused as a whole: nothing else in the body was written either.
assert user_profile.phone != "+14155550100"
@pytest.mark.django_db
@pytest.mark.parametrize("verb", VERBS)
def test_member_cannot_change_their_own_email(
verb, user_client, regular_user, user_profile
):
response = getattr(user_client, verb)(
_url(regular_user), {"email": "me-now@test.com"}, format="json"
)
assert response.status_code == status.HTTP_400_BAD_REQUEST
regular_user.refresh_from_db()
assert regular_user.email == "user@test.com"
@pytest.mark.django_db
@pytest.mark.parametrize("verb", VERBS)
def test_admin_cannot_change_their_own_email(verb, admin_client, admin_user):
response = getattr(admin_client, verb)(
_url(admin_user), {"email": "boss-now@test.com"}, format="json"
)
assert response.status_code == status.HTTP_400_BAD_REQUEST
admin_user.refresh_from_db()
assert admin_user.email == "admin@test.com"
@pytest.mark.django_db
@pytest.mark.parametrize("verb", VERBS)
@pytest.mark.parametrize("echoed", ["user@test.com", "USER@Test.com"])
def test_unchanged_email_is_accepted_and_other_fields_update(
verb, echoed, admin_client, regular_user, user_profile
):
response = getattr(admin_client, verb)(
_url(regular_user),
{"email": echoed, "role": "ADMIN", "phone": "+14155550100"},
format="json",
)
assert response.status_code == status.HTTP_200_OK, response.data
regular_user.refresh_from_db()
user_profile.refresh_from_db()
# The stored spelling is kept, not the echoed one.
assert regular_user.email == "user@test.com"
assert user_profile.phone == "+14155550100"
assert user_profile.role == "ADMIN"
@pytest.mark.django_db
def test_patch_without_email_still_updates(admin_client, regular_user, user_profile):
response = admin_client.patch(
_url(regular_user), {"phone": "+14155550100"}, format="json"
)
assert response.status_code == status.HTTP_200_OK, response.data
user_profile.refresh_from_db()
regular_user.refresh_from_db()
assert user_profile.phone == "+14155550100"
assert regular_user.email == "user@test.com"
@pytest.mark.django_db
def test_self_profile_update_ignores_an_email(user_client, regular_user):
"""`PATCH /api/profile/` names only phone and name; an email is not written."""
user_client.patch("/api/profile/", {"email": "me-now@test.com"}, format="json")
regular_user.refresh_from_db()
assert regular_user.email == "user@test.com"
@pytest.mark.django_db
def test_inviting_a_new_member_still_sets_their_email(admin_client, org_a):
from common.models import Profile
response = admin_client.post(
"/api/users/", {"email": "New.Person@test.com", "role": "USER"}, format="json"
)
assert response.status_code == status.HTTP_201_CREATED, response.data
assert Profile.objects.filter(org=org_a, user__email="new.person@test.com").exists()
# ── the rest of the account row: profile_pic ─────────────────────────────
#
# `User` is shared by every org a person belongs to, so an admin editing
# another member may not change any field on it (coordinator decision,
# 1.11.0). `CreateUserSerializer` names two account fields, email (above) and
# profile_pic (below). Profile fields on the same request are unaffected.
PIC = "https://cdn.test/me.png"
@pytest.mark.django_db
@pytest.mark.parametrize("verb", VERBS)
def test_admin_cannot_change_another_members_picture(
verb, admin_client, regular_user, user_profile
):
regular_user.profile_pic = PIC
regular_user.save(update_fields=["profile_pic"])
response = getattr(admin_client, verb)(
_url(regular_user),
{
"email": regular_user.email,
"profile_pic": "https://evil.test/x.png",
"phone": "+14155550100",
},
format="json",
)
assert response.status_code == status.HTTP_400_BAD_REQUEST
assert "profile_pic" in str(response.data)
regular_user.refresh_from_db()
user_profile.refresh_from_db()
assert regular_user.profile_pic == PIC
assert user_profile.phone != "+14155550100"
@pytest.mark.django_db
@pytest.mark.parametrize("verb", VERBS)
def test_admin_resending_the_stored_picture_is_accepted(
verb, admin_client, regular_user, user_profile
):
regular_user.profile_pic = PIC
regular_user.save(update_fields=["profile_pic"])
response = getattr(admin_client, verb)(
_url(regular_user),
{"email": regular_user.email, "profile_pic": PIC, "role": "ADMIN"},
format="json",
)
assert response.status_code == status.HTTP_200_OK, response.data
regular_user.refresh_from_db()
user_profile.refresh_from_db()
assert regular_user.profile_pic == PIC
assert user_profile.role == "ADMIN"
@pytest.mark.django_db
@pytest.mark.parametrize("verb", VERBS)
def test_a_member_may_change_their_own_picture(verb, user_client, regular_user):
response = getattr(user_client, verb)(
_url(regular_user),
{"email": regular_user.email, "profile_pic": PIC},
format="json",
)
assert response.status_code == status.HTTP_200_OK, response.data
regular_user.refresh_from_db()
assert regular_user.profile_pic == PIC
@pytest.mark.django_db
def test_the_serializer_fails_closed_when_the_caller_is_not_named(regular_user, org_a):
from common.serializer import CreateUserSerializer
serializer = CreateUserSerializer(
data={"email": regular_user.email, "profile_pic": PIC},
instance=regular_user,
org=org_a,
)
assert not serializer.is_valid()
assert "profile_pic" in serializer.errors

View file

@ -0,0 +1,56 @@
"""A vertical pack may not define a ticket stage that maps to Duplicate.
A ticket becomes Duplicate only by being merged. `CaseStageSerializer` refuses
such a stage when one is made through the API, but a pack writes its stages
without that serializer, so the manifest is refused at validation instead.
"""
import pytest
from cases.workflow import DUPLICATE_BY_MERGE_ONLY
from common.packs.schema import PackValidationError, validate_manifest
def _case_pack(maps_to_status):
return {
"id": "demo",
"version": 1,
"name": "Demo",
"case_pipeline": {
"name": "Support",
"stages": [
{"name": "Open"},
{"name": "Dupes", "maps_to_status": maps_to_status},
],
},
}
def test_a_case_stage_mapped_to_duplicate_is_refused():
with pytest.raises(PackValidationError) as exc:
validate_manifest(_case_pack("Duplicate"))
message = str(exc.value)
assert (
"case_pipeline.stages[1]: maps_to_status 'Duplicate' is not allowed" in message
)
assert DUPLICATE_BY_MERGE_ONLY in message
@pytest.mark.parametrize("status", ["Closed", "Pending", None])
def test_a_case_stage_mapped_to_any_other_status_is_accepted(status):
raw = _case_pack(status)
assert validate_manifest(raw) is raw
def test_a_task_stage_named_duplicate_is_not_this_rule():
"""Task stages have no status vocabulary; the rule is about tickets."""
raw = {
"id": "demo",
"version": 1,
"name": "Demo",
"task_pipeline": {
"name": "Work",
"stages": [{"name": "Dupes", "maps_to_status": "Duplicate"}],
},
}
assert validate_manifest(raw) is raw

View file

@ -1,5 +1,5 @@
"""`client_ip` has to return an IP or nothing, never a string that merely
looks like one.
looks like one, and never an address the caller chose.
`WebFormSubmission.submitted_ip` is a GenericIPAddressField, which maps to a
Postgres `inet` column. Django does not run field validators on `save()`, so an
@ -17,24 +17,49 @@ def _request(**meta):
class TestClientIp:
def test_prefers_the_first_forwarded_entry(self):
"""Only the entries our own proxies appended are believed. See
`common.request_meta.client_ip`."""
def test_with_no_proxy_configured_the_header_is_ignored(self):
request = _request(
HTTP_X_FORWARDED_FOR="203.0.113.50, 70.41.3.18", REMOTE_ADDR="10.0.0.1"
)
assert client_ip(request) == "10.0.0.1"
def test_behind_one_proxy_the_entry_it_appended_is_used(self, settings):
settings.REST_FRAMEWORK = {**settings.REST_FRAMEWORK, "NUM_PROXIES": 1}
request = _request(
HTTP_X_FORWARDED_FOR="6.6.6.6, 203.0.113.50", REMOTE_ADDR="127.0.0.1"
)
assert client_ip(request) == "203.0.113.50"
def test_falls_back_to_remote_addr(self):
def test_behind_two_proxies_the_outer_ones_entry_is_used(self, settings):
settings.REST_FRAMEWORK = {**settings.REST_FRAMEWORK, "NUM_PROXIES": 2}
request = _request(
HTTP_X_FORWARDED_FOR="6.6.6.6, 203.0.113.50, 10.0.0.2",
REMOTE_ADDR="127.0.0.1",
)
assert client_ip(request) == "203.0.113.50"
def test_a_short_header_uses_its_leftmost_entry(self, settings):
settings.REST_FRAMEWORK = {**settings.REST_FRAMEWORK, "NUM_PROXIES": 2}
request = _request(HTTP_X_FORWARDED_FOR="203.0.113.50", REMOTE_ADDR="10.0.0.2")
assert client_ip(request) == "203.0.113.50"
def test_behind_a_proxy_with_no_header_the_peer_is_used(self, settings):
settings.REST_FRAMEWORK = {**settings.REST_FRAMEWORK, "NUM_PROXIES": 1}
assert client_ip(_request(REMOTE_ADDR="10.0.0.1")) == "10.0.0.1"
def test_accepts_ipv6(self):
request = _request(HTTP_X_FORWARDED_FOR="2001:db8::1", REMOTE_ADDR="10.0.0.1")
assert client_ip(request) == "2001:db8::1"
assert client_ip(_request(REMOTE_ADDR="2001:db8::1")) == "2001:db8::1"
def test_skips_a_junk_forwarded_entry_and_uses_the_next_candidate(self):
def test_a_junk_trusted_entry_is_none_not_a_fallback(self, settings):
"""Falling back to another entry would let the caller's text through."""
settings.REST_FRAMEWORK = {**settings.REST_FRAMEWORK, "NUM_PROXIES": 1}
request = _request(
HTTP_X_FORWARDED_FOR="not-an-ip, 203.0.113.50", REMOTE_ADDR="10.0.0.1"
HTTP_X_FORWARDED_FOR="203.0.113.50, not-an-ip", REMOTE_ADDR="10.0.0.1"
)
assert client_ip(request) == "203.0.113.50"
assert client_ip(request) is None
def test_returns_none_when_nothing_validates(self):
request = _request(HTTP_X_FORWARDED_FOR="drop table students", REMOTE_ADDR="")
@ -45,9 +70,10 @@ class TestClientIp:
request.META.pop("REMOTE_ADDR", None)
assert client_ip(request) is None
def test_a_sql_injection_payload_never_reaches_the_caller(self):
def test_a_sql_injection_payload_never_reaches_the_caller(self, settings):
"""The return value is written straight into an `inet` column, so a
non-IP getting through here is a 500 at best."""
settings.REST_FRAMEWORK = {**settings.REST_FRAMEWORK, "NUM_PROXIES": 1}
request = _request(
HTTP_X_FORWARDED_FOR="1.1.1.1'; DROP TABLE lead; --", REMOTE_ADDR=""
)

View file

@ -0,0 +1,354 @@
"""Saved list views (G29): private to their owner, and only what the list reads.
Run with: pytest common/tests/test_saved_views.py -v
"""
import uuid
from types import SimpleNamespace
import pytest
from django.http import QueryDict
from rest_framework import status as http
from common.models import Profile, SavedView, User
from common.saved_views import LISTS, MAX_VIEWS_PER_LIST
from common.testing import _make_authenticated_client
URL = "/api/saved-views/"
def detail(pk):
return f"{URL}{pk}/"
def create(client, **body):
payload = {"module": "leads", "name": "Hot", "filters": {}}
payload.update(body)
return client.post(URL, payload, format="json")
@pytest.fixture
def colleague(org_a):
"""A second member of org A, with a client of their own."""
user = User.objects.create_user(email="colleague@test.com", password="x")
profile = Profile.objects.create(user=user, org=org_a, role="USER", is_active=True)
return SimpleNamespace(
profile=profile, client=_make_authenticated_client(user, org_a, profile)
)
# ---------------------------------------------------------------- owner CRUD
def test_owner_creates_lists_reads_renames_and_deletes(user_client, user_profile):
tag = str(uuid.uuid4())
created = create(
user_client,
name=" Hot leads ",
filters={"status": ["assigned", "in process"], "tags": tag, "search": ""},
)
assert created.status_code == http.HTTP_201_CREATED, created.data
view_id = created.data["id"]
assert created.data["name"] == "Hot leads"
# One value becomes a list of one; a blank is dropped with its key.
assert created.data["filters"] == {
"status": ["assigned", "in process"],
"tags": [tag],
}
stored = SavedView.objects.get(pk=view_id)
assert stored.profile == user_profile
assert stored.org == user_profile.org
listed = user_client.get(URL, {"module": "leads"})
assert listed.status_code == 200
assert [v["id"] for v in listed.data["saved_views"]] == [view_id]
assert listed.data["limit"] == MAX_VIEWS_PER_LIST
assert user_client.get(detail(view_id)).data["name"] == "Hot leads"
renamed = user_client.patch(detail(view_id), {"name": "Warm"}, format="json")
assert renamed.status_code == 200, renamed.data
assert renamed.data["name"] == "Warm"
assert renamed.data["filters"]["tags"] == [tag]
refiltered = user_client.patch(
detail(view_id), {"filters": {"source": "call"}}, format="json"
)
assert refiltered.status_code == 200, refiltered.data
assert refiltered.data["filters"] == {"source": ["call"]}
assert user_client.delete(detail(view_id)).status_code == 204
assert not SavedView.objects.filter(pk=view_id).exists()
def test_list_without_module_returns_every_list_and_bad_module_is_400(user_client):
create(user_client, module="leads", name="A")
create(user_client, module="cases", name="B")
everything = user_client.get(URL)
assert sorted(v["module"] for v in everything.data["saved_views"]) == [
"cases",
"leads",
]
assert user_client.get(URL, {"module": "tasks"}).status_code == 400
assert create(user_client, module="tasks").status_code == 400
def test_every_one_of_the_six_lists_takes_a_view(user_client):
for module in LISTS:
response = create(user_client, module=module, name=f"{module} view")
assert response.status_code == 201, (module, response.data)
def test_anonymous_is_refused(unauthenticated_client):
assert unauthenticated_client.get(URL).status_code in (401, 403)
assert create(unauthenticated_client).status_code in (401, 403)
assert not SavedView.objects.exists()
# ------------------------------------------------------------- private views
def test_another_members_view_is_404_on_every_verb(user_client, colleague):
theirs = create(colleague.client, name="Theirs").data["id"]
missing = str(uuid.uuid4())
# Hidden from the list, and every verb answers exactly as for a missing id.
assert user_client.get(URL).data["saved_views"] == []
for verb, kwargs in (
("get", {}),
("patch", {"data": {"name": "Mine now"}, "format": "json"}),
("delete", {}),
):
hidden = getattr(user_client, verb)(detail(theirs), **kwargs)
absent = getattr(user_client, verb)(detail(missing), **kwargs)
assert hidden.status_code == absent.status_code == 404, verb
assert hidden.data == absent.data, verb
view = SavedView.objects.get(pk=theirs)
assert view.name == "Theirs"
# The owner still reads it: the check refuses one person, not everyone.
assert colleague.client.get(detail(theirs)).status_code == 200
def test_an_admin_cannot_read_a_members_view_either(admin_client, user_client):
member_view = create(user_client, name="Private").data["id"]
assert admin_client.get(detail(member_view)).status_code == 404
assert admin_client.get(URL).data["saved_views"] == []
def test_another_orgs_view_is_invisible(user_client, org_b_client):
other = create(org_b_client, name="Org B view").data["id"]
assert user_client.get(URL).data["saved_views"] == []
assert user_client.get(detail(other)).status_code == 404
assert user_client.delete(detail(other)).status_code == 404
assert SavedView.objects.filter(pk=other).exists()
def test_org_and_profile_in_the_body_are_refused(user_client, colleague, org_b):
response = create(user_client, profile=str(colleague.profile.id))
assert response.status_code == 400
assert "profile" in response.data
response = create(user_client, org=str(org_b.id))
assert response.status_code == 400
assert "org" in response.data
assert not SavedView.objects.exists()
mine = create(user_client).data["id"]
moved = user_client.patch(
detail(mine), {"profile": str(colleague.profile.id)}, format="json"
)
assert moved.status_code == 400
assert SavedView.objects.get(pk=mine).profile.user.email == "user@test.com"
def test_a_view_cannot_move_to_another_list(user_client):
mine = create(user_client).data["id"]
response = user_client.patch(detail(mine), {"module": "cases"}, format="json")
assert response.status_code == 400
assert "module" in response.data
# Naming its own list again is not a move.
same = user_client.patch(detail(mine), {"module": "leads"}, format="json")
assert same.status_code == 200
# ------------------------------------------------------------ what it holds
@pytest.mark.parametrize(
"module, filters",
[
("leads", {"limit": "10"}),
("leads", {"junk": "x"}),
("leads", {"pipeline": str(uuid.uuid4())}), # a deal param, not a lead one
("invoices", {"due_date__gte": "2026-01-01"}), # invoices use one underscore
("cases", {"cf_": "x"}),
("cases", {"cf_bad-key": "x"}),
("leads", ["status", "assigned"]),
("leads", "status=assigned"),
("leads", {"status": 5}),
("leads", {"status": ["assigned", None]}),
("leads", {"status": {"nested": "x"}}),
("leads", {"search": "x" * 201}),
("leads", {"status": ["assigned"] * 51}),
("leads", {f"cf_k{i}": "x" for i in range(31)}),
# Malformed values are refused by the list's own parsing.
("leads", {"assigned_to": "not-a-uuid"}),
("contacts", {"tags": ["x"]}),
("accounts", {"created_at__gte": "2026-02-30"}),
("opportunities", {"amount__gte": "lots"}),
("opportunities", {"amount__lte": "NaN"}),
("cases", {"account": "nope"}),
("invoices", {"due_date_gte": "yesterday"}),
],
)
def test_junk_filters_are_400(user_client, module, filters):
response = create(user_client, module=module, filters=filters)
assert response.status_code == 400, response.data
assert "filters" in response.data
assert not SavedView.objects.exists()
def test_junk_filters_are_400_on_update_too(user_client):
mine = create(user_client, filters={"source": "call"}).data["id"]
response = user_client.patch(
detail(mine), {"filters": {"assigned_to": "x"}}, format="json"
)
assert response.status_code == 400
assert SavedView.objects.get(pk=mine).filters == {"source": ["call"]}
def _sample(key):
"""A well-formed value for list parameter ``key``."""
if key in {
"assigned_to",
"tags",
"account",
"contact",
"opportunity",
"pipeline",
"created_by",
}:
return str(uuid.uuid4())
if key.startswith("amount"):
return "10.5"
if "date" in key or "created_at" in key or "closed_on" in key:
return "2026-01-02"
if key == "next_follow_up":
return "2026-01-02"
return "true"
def test_every_param_the_lists_read_is_accepted(user_client):
for module, (_build, accepted) in LISTS.items():
filters = {key: _sample(key) for key in accepted}
filters["cf_region"] = "EMEA"
response = create(user_client, module=module, name=module, filters=filters)
assert response.status_code == 201, (module, response.data)
class _Recorder(QueryDict):
"""A query string that remembers every key it was asked for."""
def __init__(self):
super().__init__(mutable=True)
# Non-empty, so a list that returns early on no params still reads on.
self.setlist("__probe__", ["x"])
self.read = set()
def __getitem__(self, key):
self.read.add(key)
return super().__getitem__(key)
def get(self, key, default=None):
self.read.add(key)
return super().get(key, default)
def getlist(self, key, default=None):
self.read.add(key)
return super().getlist(key, default)
@pytest.mark.parametrize("module", sorted(LISTS))
def test_accepted_params_match_what_the_list_reads(module, admin_user, admin_profile):
"""The written-out sets drift from the list functions the day one of them
grows a filter. Asked both ways, so a param a list stops reading is caught
as well as one it starts reading."""
build, accepted = LISTS[module]
params = _Recorder()
build(SimpleNamespace(profile=admin_profile, user=admin_user), params)
assert params.read - {"__probe__"} == accepted
# ------------------------------------------------------------ names and cap
def test_duplicate_name_is_400_case_insensitively(user_client, colleague):
assert create(user_client, name="Hot").status_code == 201
duplicate = create(user_client, name=" hot ")
assert duplicate.status_code == 400
assert "name" in duplicate.data
# Another list, or another person, may use the same name.
assert create(user_client, module="cases", name="Hot").status_code == 201
assert create(colleague.client, name="Hot").status_code == 201
other = create(user_client, name="Cold").data["id"]
clash = user_client.patch(detail(other), {"name": "HOT"}, format="json")
assert clash.status_code == 400
# Renaming a view to its own name, in another case, is not a clash.
assert (
user_client.patch(detail(other), {"name": "COLD"}, format="json").status_code
== 200
)
def test_name_is_required_and_bounded(user_client):
assert create(user_client, name=" ").status_code == 400
assert create(user_client, name="x" * 101).status_code == 400
assert create(user_client, name="x" * 100).status_code == 201
body = {"module": "leads", "filters": {}}
assert user_client.post(URL, body, format="json").status_code == 400
def test_cap_per_list_is_enforced(user_client, colleague):
for i in range(MAX_VIEWS_PER_LIST):
assert create(user_client, name=f"View {i}").status_code == 201
over = create(user_client, name="One too many")
assert over.status_code == 400
assert SavedView.objects.filter(module="leads").count() == MAX_VIEWS_PER_LIST
# The cap is per person and per list.
assert create(user_client, module="cases", name="Fine").status_code == 201
assert create(colleague.client, name="Fine").status_code == 201
# A full list can still be renamed.
first = SavedView.objects.filter(name="View 0").first()
assert (
user_client.patch(
detail(first.id), {"name": "Renamed"}, format="json"
).status_code
== 200
)
def test_a_save_that_loses_the_race_is_a_400_not_a_500(user_profile, regular_user):
"""Two saves can both pass the name check; the constraint catches the second."""
from rest_framework.exceptions import ValidationError
from common.saved_views import SavedViewSerializer
request = SimpleNamespace(profile=user_profile, user=regular_user)
serializer = SavedViewSerializer(
data={"module": "leads", "name": "Hot"}, context={"request": request}
)
assert serializer.is_valid(), serializer.errors
SavedView.objects.create(
org=user_profile.org, profile=user_profile, module="leads", name="HOT"
)
with pytest.raises(ValidationError) as caught:
serializer.save(org=user_profile.org, profile=user_profile)
assert "name" in caught.value.detail
assert SavedView.objects.count() == 1
def test_a_refused_value_reads_as_one_sentence_per_param(user_client):
response = create(user_client, filters={"assigned_to": "nope"})
assert response.status_code == 400
assert response.data["filters"] == ["assigned_to: 'nope' is not a valid id."]

View file

@ -0,0 +1,290 @@
"""A Django superuser is an org admin in every org they are a member of.
Owner decision for 1.11.0. ``is_org_admin`` read ``role`` alone while dozens of
call sites added ``or request.user.is_superuser`` by hand and the rest did not,
and both clients gated admin UI on ``role == "ADMIN"``, so a superuser holding
the USER role saw read-only settings pages the API would have let them change.
What is pinned here:
* the rule itself, both ways, on real profiles;
* the one fact the clients gate on (``is_organization_admin`` in the JWT, the
org lists and the profile payloads) agrees with the rule;
* a representative admin-only endpoint admits the superuser and refuses a
plain member;
* membership is still required, and neither ``is_superuser``, ``is_staff`` nor
``role`` can be written by the caller through the API.
"""
from unittest.mock import MagicMock
import pytest
from rest_framework import status
from rest_framework.test import APIClient
from rest_framework_simplejwt.tokens import AccessToken
from common.models import Profile, User
from common.permissions import IsOrgAdmin, is_org_admin
from common.serializer import OrgAwareRefreshToken
from common.testing import _make_authenticated_client
from common.views.auth_views import _org_payload
@pytest.fixture
def super_user():
user = User.objects.create_user(email="root@test.com", password="testpass123")
user.is_superuser = True
user.is_staff = True
user.save()
return user
@pytest.fixture
def super_profile(super_user, org_a):
return Profile.objects.create(
user=super_user, org=org_a, role="USER", is_active=True
)
@pytest.fixture
def super_client(super_user, org_a, super_profile):
return _make_authenticated_client(super_user, org_a, super_profile)
@pytest.mark.django_db
class TestTheRule:
def test_admin_role_is_admin(self, admin_profile):
assert is_org_admin(admin_profile) is True
def test_user_role_superuser_is_admin(self, super_profile):
assert is_org_admin(super_profile) is True
def test_user_role_member_is_not(self, user_profile):
assert is_org_admin(user_profile) is False
def test_permission_class_agrees(self, super_profile, user_profile):
class _Request:
def __init__(self, profile):
self.profile = profile
assert IsOrgAdmin().has_permission(_Request(super_profile), None) is True
assert IsOrgAdmin().has_permission(_Request(user_profile), None) is False
def test_profile_is_admin_property_delegates(self, super_profile, user_profile):
assert super_profile.is_admin is True
assert user_profile.is_admin is False
def test_a_truthy_non_bool_is_not_a_superuser(self):
"""A test double answers every attribute with a truthy Mock."""
profile = MagicMock()
profile.role = "USER"
assert is_org_admin(profile) is False
def test_the_column_still_mirrors_the_role_only(self, super_profile):
"""`is_organization_admin` the column is not the fact; the API field is."""
super_profile.refresh_from_db()
assert super_profile.is_organization_admin is False
@pytest.mark.django_db
class TestThePayloadFactMatchesTheRule:
def _claim(self, user, org, profile):
token = OrgAwareRefreshToken.for_user_and_org(user, org, profile)
return AccessToken(str(token.access_token)).get("is_organization_admin")
def test_jwt_claim(
self,
org_a,
super_user,
super_profile,
admin_user,
admin_profile,
regular_user,
user_profile,
):
assert self._claim(super_user, org_a, super_profile) is True
assert self._claim(admin_user, org_a, admin_profile) is True
assert self._claim(regular_user, org_a, user_profile) is False
def test_org_payload(self, org_a, super_profile, user_profile):
assert _org_payload(org_a, profile=super_profile)["is_organization_admin"]
member = _org_payload(org_a, profile=user_profile)
assert member["is_organization_admin"] is False
assert member["role"] == "USER"
assert "is_organization_admin" not in _org_payload(org_a)
def test_profile_endpoint(self, super_client, user_client):
assert super_client.get("/api/auth/profile/").data["is_organization_admin"]
member = user_client.get("/api/auth/profile/").data
assert member["is_organization_admin"] is False
def test_me_endpoint(self, super_client, user_client, org_a):
orgs = super_client.get("/api/auth/me/").data["organizations"]
assert [o["is_organization_admin"] for o in orgs] == [True]
orgs = user_client.get("/api/auth/me/").data["organizations"]
assert [o["is_organization_admin"] for o in orgs] == [False]
def test_org_list_endpoint(self, super_client, user_client):
rows = super_client.get("/api/org/").data["profile_org_list"]
assert [r["is_organization_admin"] for r in rows] == [True]
rows = user_client.get("/api/org/").data["profile_org_list"]
assert [r["is_organization_admin"] for r in rows] == [False]
def test_switch_org_response_and_new_token(
self, super_user, super_profile, org_a, regular_user, user_profile
):
for user, expected in ((super_user, True), (regular_user, False)):
client = APIClient()
token = OrgAwareRefreshToken.for_user_and_org(user, None)
client.credentials(HTTP_AUTHORIZATION=f"Bearer {token.access_token}")
response = client.post(
"/api/auth/switch-org/", {"org_id": str(org_a.id)}, format="json"
)
assert response.status_code == status.HTTP_200_OK, response.data
assert response.data["profile"]["is_organization_admin"] is expected
claims = AccessToken(response.data["access_token"])
assert claims["is_organization_admin"] is expected
@pytest.mark.django_db
class TestAnAdminOnlyEndpoint:
def test_api_settings_create_admits_a_user_role_superuser(self, super_client):
response = super_client.post(
"/api/api-settings/",
{"title": "Site", "website": "https://site.com"},
format="json",
)
assert response.status_code == status.HTTP_201_CREATED, response.data
def test_api_settings_create_refuses_a_member(self, user_client):
response = user_client.post(
"/api/api-settings/",
{"title": "Site", "website": "https://site.com"},
format="json",
)
assert response.status_code == status.HTTP_403_FORBIDDEN
def test_pack_sample_data_gate_admits_superuser_and_refuses_member(
self, super_client, user_client
):
"""`_require_admin` in pack_views used to compare `role` inline."""
assert (
super_client.delete("/api/packs/sample-data/").status_code
!= status.HTTP_403_FORBIDDEN
)
assert (
user_client.delete("/api/packs/sample-data/").status_code
== status.HTTP_403_FORBIDDEN
)
@pytest.mark.django_db
class TestNoEscalation:
def test_superuser_without_a_profile_cannot_enter_another_org(
self, super_user, super_profile, org_b
):
"""Superuser is admin in orgs they belong to; it is not a pass into others."""
client = APIClient()
token = OrgAwareRefreshToken.for_user_and_org(super_user, None)
client.credentials(HTTP_AUTHORIZATION=f"Bearer {token.access_token}")
response = client.post(
"/api/auth/switch-org/", {"org_id": str(org_b.id)}, format="json"
)
assert response.status_code == status.HTTP_403_FORBIDDEN
def test_an_inactive_superuser_membership_gets_no_org_context(
self, super_user, super_profile, org_a
):
client = _make_authenticated_client(super_user, org_a, super_profile)
super_profile.is_active = False
super_profile.save()
response = client.post(
"/api/api-settings/",
{"title": "Site", "website": "https://site.com"},
format="json",
)
assert response.status_code == status.HTTP_403_FORBIDDEN
def test_member_cannot_make_themselves_superuser_or_admin(
self, user_client, regular_user, user_profile
):
body = {
"is_superuser": True,
"is_staff": True,
"role": "ADMIN",
"is_organization_admin": True,
"name": "Still Me",
}
user_client.patch("/api/profile/", body, format="json")
user_client.put(f"/api/user/{regular_user.id}/", body, format="json")
regular_user.refresh_from_db()
user_profile.refresh_from_db()
assert regular_user.is_superuser is False
assert regular_user.is_staff is False
assert user_profile.role == "USER"
assert is_org_admin(user_profile) is False
def test_admin_cannot_make_a_member_superuser(
self, admin_client, regular_user, user_profile
):
admin_client.put(
f"/api/user/{regular_user.id}/",
{
"email": regular_user.email,
"role": "USER",
"is_superuser": True,
"is_staff": True,
},
format="json",
)
regular_user.refresh_from_db()
assert regular_user.is_superuser is False
assert regular_user.is_staff is False
def test_superuser_cannot_change_their_own_role(
self, super_client, super_user, super_profile
):
"""Being an admin does not lift the no-self-role-change rule."""
response = super_client.put(
f"/api/user/{super_user.id}/",
{"email": super_user.email, "role": "ADMIN"},
format="json",
)
assert response.status_code == status.HTTP_400_BAD_REQUEST
super_profile.refresh_from_db()
assert super_profile.role == "USER"
@pytest.mark.django_db
def test_an_admin_approval_rule_counts_a_superuser_as_an_admin(
org_a, super_profile, user_profile, admin_profile
):
"""`Approval.can_be_acted_on_by` compared `role` to the rule's role inline."""
from cases.approvals import Approval, ApprovalRule
rule = ApprovalRule.objects.create(org=org_a, name="Admins", approver_role="ADMIN")
approval = Approval(org=org_a, rule=rule)
assert approval.can_be_acted_on_by(admin_profile) is True
assert approval.can_be_acted_on_by(super_profile) is True
assert approval.can_be_acted_on_by(user_profile) is False
@pytest.mark.django_db
def test_org_create_response_carries_the_creators_admin_fact(regular_user):
"""Mobile caches the created org straight from this response; without the
fact the creator saw no admin controls until they signed in again."""
client = APIClient()
token = OrgAwareRefreshToken.for_user_and_org(regular_user, None)
client.credentials(HTTP_AUTHORIZATION=f"Bearer {token.access_token}")
response = client.post(
"/api/org/", {"name": "Brand New Org", "is_organization_admin": False}
)
assert response.status_code == status.HTTP_200_OK, response.data
org = response.data["org"]
assert org["role"] == "ADMIN"
assert org["is_organization_admin"] is True
assert Profile.objects.get(user=regular_user, org_id=org["id"]).role == "ADMIN"

View file

@ -1,6 +1,7 @@
from django.urls import path
from common.views.attachment_views import AttachmentDownloadView
from common.views.audit_log_views import SecurityAuditLogListView
from common.views.auth_views import (
GoogleIdTokenView,
GoogleOAuthCallbackView,
@ -47,6 +48,7 @@ from common.views.pat_views import (
PersonalAccessTokenDetailView,
PersonalAccessTokenListCreateView,
)
from common.views.saved_view_views import SavedViewDetailView, SavedViewListView
from common.views.settings_views import DomainDetailView, DomainList
from common.views.tags_views import (
TagsDetailView,
@ -117,6 +119,7 @@ urlpatterns = [
name="help_center_settings",
),
path("org/tokens/", OrgAccessTokenListView.as_view(), name="org_pat_list"),
path("org/audit-log/", SecurityAuditLogListView.as_view(), name="org_audit_log"),
path(
"org/tokens/<uuid:pk>/",
OrgAccessTokenDetailView.as_view(),
@ -188,6 +191,13 @@ urlpatterns = [
NotificationDetailView.as_view(),
name="notifications_detail",
),
# A profile's own saved list filters (G29).
path("saved-views/", SavedViewListView.as_view(), name="saved_view_list"),
path(
"saved-views/<uid:pk>/",
SavedViewDetailView.as_view(),
name="saved_view_detail",
),
# Vertical packs: any member may list; apply/clear are ADMIN-only (see
# common/views/pack_views.py). sample-data/ must precede
# <str:pack_id>/apply/ so it is never captured as a pack id.

View file

@ -13,78 +13,67 @@ therefore offering a download that could not work and, where it did work,
worked for the wrong people.
**Reading an attachment is reading the record it hangs off**, so this view
asks that record's own read predicate rather than inventing a second one.
Every one of the seven predicates below already exists and is already used by
that model's detail view. A content type not in the map is refused: a new
attachable model must opt in here deliberately, because the failure mode of
the other default is handing out somebody's file.
asks that record's own read rule rather than inventing a second one: each
module's `visible_*_qs`, the queryset its list and detail views use. A file on
a record the caller cannot open answers the same 404 as an id that does not
exist, so the download cannot confirm the record or the file is there. A
content type not in the map, and an attachment whose record is gone, answer
that 404 too: a new attachable model must opt in here deliberately, because
the failure mode of the other default is handing out somebody's file.
"""
from django.http import FileResponse, Http404
from django.shortcuts import get_object_or_404
from drf_spectacular.types import OpenApiTypes
from drf_spectacular.utils import extend_schema
from rest_framework import status
from rest_framework.permissions import IsAuthenticated
from rest_framework.response import Response
from rest_framework.views import APIView
from common import swagger_params
from common.lookups import get_on_visible_record_or_404, get_scoped_or_404
from common.models import Attachments
from common.permissions import HasOrgContext
def _readers():
"""content_type.model -> a predicate answering "may this caller read it".
def _visible():
"""content_type.model -> the records of that kind the caller may open.
Built lazily inside the function because these modules import from
`common`, and importing them at `common.views` module level would close
the circle.
"""
from accounts.access import has_account_access
from cases.access import has_case_read_access
from contacts.access import has_contact_access
from invoices.permissions import has_object_access
from leads.access import has_lead_access
from opportunity.access import has_deal_access
from tasks.access import has_task_access
from accounts.access import visible_accounts_qs
from cases.access import visible_cases_qs
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
from tasks.access import visible_tasks_qs
return {
"lead": lambda request, obj: has_lead_access(
request.profile, request.user, obj
),
"opportunity": lambda request, obj: has_deal_access(
request.profile, request.user, obj
),
"contact": lambda request, obj: has_contact_access(request.profile, obj),
"account": lambda request, obj: has_account_access(
request.profile, request.user, obj
),
"case": lambda request, obj: has_case_read_access(request.profile, obj),
"task": lambda request, obj: has_task_access(request.profile, obj),
"invoice": has_object_access,
"lead": lambda request: visible_leads_qs(request.profile, request.user),
"opportunity": lambda request: visible_deals_qs(request.profile, request.user),
"contact": lambda request: visible_contacts_qs(request.profile),
"account": lambda request: visible_accounts_qs(request.profile, request.user),
"case": lambda request: visible_cases_qs(request.profile),
"task": lambda request: visible_tasks_qs(request.profile),
"invoice": lambda request: visible_invoices_qs(request.profile, request.user),
}
def may_read_attachment(request, attachment):
"""True when the caller may read the record this file is attached to.
def get_readable_attachment_or_404(request, pk):
"""The attachment ``pk`` if the caller may open the record it hangs off.
Deny by default, twice over. An unmapped content type is refused, and so
is an attachment whose parent row has been deleted out from under it: a
dangling `object_id` is not an absence of a rule, it is a record nobody
can be checked against.
Raises the same 404 for a missing id, another org's file, a file on a
record the caller cannot read, an unmapped content type and a record that
has been deleted: the second lookup only finds the row through the
module's read-rule queryset, which holds none of those.
"""
reader = _readers().get(attachment.content_type.model)
if reader is None:
return False
parent = attachment.content_object
if parent is None:
return False
# The org filter on the attachment does not cover the parent, and a
# ContentType lookup crosses tenants freely, so check it here as well.
if getattr(parent, "org_id", None) != request.profile.org_id:
return False
return reader(request, parent)
org = request.profile.org
found = get_scoped_or_404(Attachments, pk, org)
visible = _visible().get(found.content_type.model)
if visible is None:
raise Http404(f"No such {Attachments._meta.verbose_name}.")
return get_on_visible_record_or_404(Attachments, pk, org, visible(request))
class AttachmentDownloadView(APIView):
@ -99,19 +88,7 @@ class AttachmentDownloadView(APIView):
responses={(200, "application/octet-stream"): OpenApiTypes.BINARY},
)
def get(self, request, pk, format=None):
attachment = get_object_or_404(
Attachments.objects.select_related("content_type"),
id=pk,
org=request.profile.org,
)
if not may_read_attachment(request, attachment):
return Response(
{
"error": True,
"errors": "You do not have Permission to perform this action",
},
status=status.HTTP_403_FORBIDDEN,
)
attachment = get_readable_attachment_or_404(request, pk)
if not attachment.attachment:
raise Http404("That attachment has no file.")
attachment.attachment.open("rb")

View file

@ -0,0 +1,133 @@
"""The security audit log, read by an org's admins (G33).
GET /api/org/audit-log/ newest first, paginated (limit/offset, at most 100)
?event_type=<one of SecurityAuditLog.EVENT_TYPES>
?actor=<user id>
?from=YYYY-MM-DD ?to=YYYY-MM-DD inclusive, in the org's timezone
`security_audit_log` has no RLS policy, on purpose (`common/0036`, and
`common/tests/test_audit_log_not_exposed.py`): its `org` is nullable because
the events it exists for, failed logins and invalid API keys, often have none.
So the `org=request.profile.org` filter below is the only tenant barrier, and a
row with no org never matches it.
What leaves the server is chosen field by field rather than dumped:
* `description` is not returned. An ORG_SWITCH row's text names the org the
user switched from, which is another tenant.
* `metadata` is cut down to `SAFE_DETAILS`, keys whose values are ids, counts
or sentences the server wrote. Other keys can hold what a caller supplied:
`suspicious_activity(details=...)`, the email a failed login tried, an API
key prefix.
* A path under `/api/public/` can carry a bearer token in the URL (the CSAT
link does), so only that prefix is shown.
Admins only. `/api/org/audit-log/` is on the credential deny-list in
`common/scopes.py`, so a personal access token or the org API key is refused
before it gets here, the same way `/api/webhooks/` is.
"""
from drf_spectacular.types import OpenApiTypes
from drf_spectacular.utils import OpenApiParameter, extend_schema
from rest_framework.exceptions import ValidationError
from rest_framework.pagination import LimitOffsetPagination
from rest_framework.permissions import IsAuthenticated
from rest_framework.views import APIView
from common.audit_log import SecurityAuditLog
from common.permissions import HasOrgContext, IsOrgAdmin
from common.validators import date_param, uuid_param
SAFE_DETAILS = (
"action",
"resource",
"deleted_count",
"endpoint_id",
"creator_id",
"pause_reason",
"previous_creator_id",
"changed",
)
PUBLIC_PREFIX = "/api/public/"
EVENT_LABELS = dict(SecurityAuditLog.EVENT_TYPES)
class AuditLogPagination(LimitOffsetPagination):
default_limit = 25
max_limit = 100
def _entry(row):
user = row.user
path = row.request_path
if path.startswith(PUBLIC_PREFIX):
path = PUBLIC_PREFIX
return {
"id": str(row.id),
"event_type": row.event_type,
"event_label": EVENT_LABELS.get(row.event_type, row.event_type),
"created_at": row.created_at.isoformat(),
"success": row.success,
"actor": (
{"id": str(user.id), "name": user.name, "email": user.email}
if user is not None
else None
),
"ip_address": row.ip_address,
"user_agent": row.user_agent,
"request_method": row.request_method,
"request_path": path,
"details": {
key: row.metadata[key]
for key in SAFE_DETAILS
if isinstance(row.metadata, dict) and key in row.metadata
},
}
class SecurityAuditLogListView(APIView):
permission_classes = (IsAuthenticated, HasOrgContext, IsOrgAdmin)
@extend_schema(
tags=["Audit log"],
operation_id="org_audit_log",
parameters=[
OpenApiParameter("event_type", str, OpenApiParameter.QUERY),
OpenApiParameter("actor", OpenApiTypes.UUID, OpenApiParameter.QUERY),
OpenApiParameter("from", OpenApiTypes.DATE, OpenApiParameter.QUERY),
OpenApiParameter("to", OpenApiTypes.DATE, OpenApiParameter.QUERY),
],
responses=OpenApiTypes.OBJECT,
)
def get(self, request):
params = request.query_params
rows = SecurityAuditLog.objects.filter(org=request.profile.org)
event_type = params.get("event_type")
if event_type:
if event_type not in EVENT_LABELS:
raise ValidationError({"event_type": ["Unknown event type."]})
rows = rows.filter(event_type=event_type)
actor = uuid_param(params, "actor")
if actor:
rows = rows.filter(user_id=actor)
start = date_param(params, "from")
end = date_param(params, "to")
if start and end and start > end:
raise ValidationError({"from": ["Must be on or before 'to'."]})
if start:
rows = rows.filter(created_at__date__gte=start)
if end:
rows = rows.filter(created_at__date__lte=end)
rows = rows.select_related("user").order_by("-created_at", "-id")
paginator = AuditLogPagination()
page = paginator.paginate_queryset(rows, request, view=self)
response = paginator.get_paginated_response([_entry(row) for row in page])
response.data["event_types"] = [
{"value": value, "label": label}
for value, label in SecurityAuditLog.EVENT_TYPES
]
return response

View file

@ -17,13 +17,15 @@ from rest_framework_simplejwt.authentication import JWTAuthentication
from common import serializer
from common.models import Org, Profile, User
from common.permissions import is_org_admin
from common.request_meta import client_ip
from common.serializer import OrgAwareRefreshToken
from common.utils import CURRENCY_SYMBOLS
logger = logging.getLogger(__name__)
def _org_payload(org, role=None):
def _org_payload(org, profile=None):
"""The org record every auth response hands a client.
Currency belongs here because a client picks it up at sign-in and holds it
@ -35,6 +37,11 @@ def _org_payload(org, role=None):
``default_currency`` and ``timezone`` are normalised rather than passed
through, so a blank column answers with the same "USD" and "UTC" the rest of
the codebase assumes.
With ``profile`` (the caller's membership of ``org``) it also carries
``role`` and ``is_organization_admin``. The mobile client gates admin UI on
the latter, which is ``is_org_admin`` and so admits a superuser's profile;
``role`` is for display.
"""
currency = org.default_currency or "USD"
payload = {
@ -47,8 +54,9 @@ def _org_payload(org, role=None):
# computed it instead of guessing from the device.
"timezone": org.timezone or "UTC",
}
if role is not None:
payload["role"] = role
if profile is not None:
payload["role"] = profile.role
payload["is_organization_admin"] = is_org_admin(profile)
return payload
@ -323,9 +331,9 @@ class GoogleIdTokenView(APIView):
# requires `is_active=True`, so listing a deactivated membership here
# offered an org that answers 403 the moment it is chosen.
profiles = Profile.objects.filter(user=user, is_active=True).select_related(
"org"
"org", "user"
)
organizations = [_org_payload(p.org, role=p.role) for p in profiles]
organizations = [_org_payload(p.org, profile=p) for p in profiles]
# Generate JWT token
token = OrgAwareRefreshToken.for_user_and_org(user, None)
@ -685,7 +693,7 @@ class OrgSwitchView(APIView):
"profile": {
"id": str(profile.id),
"role": profile.role,
"is_organization_admin": profile.is_organization_admin,
"is_organization_admin": is_org_admin(profile),
},
},
status=status.HTTP_200_OK,
@ -754,7 +762,7 @@ class MagicLinkRequestView(APIView):
delivery=delivery,
code_hash=code_hash,
expires_at=timezone.now() + timedelta(minutes=10),
ip_address=request.META.get("REMOTE_ADDR"),
ip_address=client_ip(request),
)
# Send email via Celery. Pass raw_code only when delivery is "code".
@ -981,12 +989,12 @@ class MagicLinkVerifyCodeView(APIView):
profiles = list(
Profile.objects.filter(user=user, is_active=True)
.select_related("org")
.select_related("org", "user")
.order_by("org__name")
)
# Same shape the Google flow returns, so a client can offer the same
# picker whichever way the user signed in.
organizations = [_org_payload(p.org, role=p.role) for p in profiles]
organizations = [_org_payload(p.org, profile=p) for p in profiles]
# Bind the session to an org only when there is no choice to make.
# Picking `profiles.first()` out of several was an arbitrary answer to

View file

@ -153,7 +153,7 @@ class ApiHomeView(APIView):
# 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.
# account assignment.
accounts = _readable(
Account.objects.filter(is_active=True, org=org),
visible_accounts_qs(profile, user),
@ -173,7 +173,7 @@ class ApiHomeView(APIView):
tasks = _readable(Task.objects.filter(org=org), visible_tasks_qs(profile))
# Decides only whether the goal summary below includes org-wide goals.
is_admin = is_org_admin(profile) or user.is_superuser
is_admin = is_org_admin(profile)
# 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

View file

@ -80,7 +80,7 @@ class DocumentListView(APIView, LimitOffsetPagination):
queryset = self.model.objects.filter(org=self.request.profile.org).order_by(
"-id"
)
if not (self.request.user.is_superuser or is_org_admin(self.request.profile)):
if not (is_org_admin(self.request.profile)):
queryset = queryset.filter(_visible_to(self.request.profile)).distinct()
request_post = params
@ -255,7 +255,7 @@ class DocumentDetailView(APIView):
who uploaded it, or an admin. `created_by` is a User FK. Comparing it
to a Profile is the bug that made this branch unreachable.
"""
if is_org_admin(self.request.profile) or self.request.user.is_superuser:
if is_org_admin(self.request.profile):
return True
return document.created_by_id == self.request.profile.user_id
@ -307,7 +307,7 @@ class DocumentDetailView(APIView):
if not self._may_read(self.object):
return self._forbidden()
profile_list = Profile.objects.filter(org=self.request.profile.org)
if is_org_admin(request.profile) or request.user.is_superuser:
if is_org_admin(request.profile):
profiles = profile_list.order_by("user__email")
else:
profiles = profile_list.filter(role="ADMIN").order_by("user__email")

View file

@ -42,7 +42,7 @@ class OrgSettingsView(APIView):
{"error": "Organization context required"},
status=status.HTTP_400_BAD_REQUEST,
)
if not is_org_admin(request.profile) and not request.user.is_superuser:
if not is_org_admin(request.profile):
return Response(
{"error": "Only admins can update organization settings"},
status=status.HTTP_403_FORBIDDEN,
@ -72,7 +72,7 @@ class HelpCenterSettingsView(APIView):
permission_classes = (IsAuthenticated, HasOrgContext)
def _can_edit(self, request):
return is_org_admin(request.profile) or request.user.is_superuser
return is_org_admin(request.profile)
def _payload(self, request, org):
data = HelpCenterSettingsSerializer(org).data

View file

@ -55,7 +55,15 @@ class OrgProfileCreateView(APIView):
{
"error": False,
"message": "New Org is Created.",
"org": self.serializer_class(org_obj).data,
# The creator's membership facts ride on the org, in the
# shape the auth endpoints use for each org they list, so a
# client caching this org gates admin UI correctly at once.
# Server-derived; nothing here is read from the request.
"org": {
**self.serializer_class(org_obj).data,
"role": profile_obj.role,
"is_organization_admin": is_org_admin(profile_obj),
},
"status": status.HTTP_201_CREATED,
}
)
@ -75,7 +83,9 @@ class OrgProfileCreateView(APIView):
"""
here we are passing profile list of the user, where org details also included
"""
profile_list = Profile.objects.filter(user=request.user)
profile_list = Profile.objects.filter(user=request.user).select_related(
"org", "user"
)
serializer = ShowOrganizationListSerializer(profile_list, many=True)
return Response(
{

View file

@ -1,7 +1,8 @@
"""Vertical-pack endpoints.
Authorization: any authenticated member may list packs; only an org ADMIN may
apply one or clear sample data. The org is always request.profile.org, never a
Authorization: any authenticated member may list packs; only an org admin
(``common.permissions.is_org_admin``: the ADMIN role, or a superuser) may apply
one or clear sample data. The org is always request.profile.org, never a
body field. Org.vertical is descriptive and is never read here for access control.
"""
@ -12,12 +13,12 @@ from rest_framework.views import APIView
from common.packs.applier import apply_pack, clear_sample_data
from common.packs.loader import get_pack, get_registry
from common.permissions import HasOrgContext
from common.permissions import HasOrgContext, is_org_admin
def _require_admin(request):
"""Return a 403 Response if the caller is not an org admin, else None."""
if getattr(request.profile, "role", None) != "ADMIN":
if not is_org_admin(getattr(request, "profile", None)):
return Response(
{"error": True, "errors": "Only an organization admin can do this."},
status=status.HTTP_403_FORBIDDEN,

View file

@ -22,6 +22,7 @@ from rest_framework.views import APIView
from common.models import Org, PortalLoginToken
from common.portal_auth import mint_portal_token
from common.request_meta import client_ip
from common.tasks import set_rls_context
from contacts.models import Contact
@ -105,7 +106,7 @@ class PortalLoginRequestView(APIView):
contact=contact,
code_hash=make_password(raw_code),
expires_at=timezone.now() + TOKEN_TTL,
ip_address=request.META.get("REMOTE_ADDR"),
ip_address=client_ip(request),
)
from common.tasks import send_portal_login_email

View file

@ -0,0 +1,107 @@
"""Saved list views (G29): a profile's named filters for one list.
Routes (under /api/saved-views/):
GET /?module=<list> the caller's own views, for one list or for all six
POST / save one
GET /<id>/ one of the caller's views
PATCH /<id>/ rename it, or replace its filters
DELETE /<id>/ delete it
A view is private to the profile that saved it, admins included: every lookup
filters on the org and the profile, so another person's id answers 404 on every
verb, the same body as an id that does not exist. What a view may hold is
checked in ``common/saved_views.py``.
"""
from django.db.models.functions import Lower
from drf_spectacular.types import OpenApiTypes
from drf_spectacular.utils import OpenApiParameter, extend_schema
from rest_framework import status
from rest_framework.permissions import IsAuthenticated
from rest_framework.response import Response
from rest_framework.views import APIView
from common.lookups import get_scoped_or_404
from common.models import SavedView
from common.permissions import HasOrgContext
from common.saved_views import LISTS, MAX_VIEWS_PER_LIST, SavedViewSerializer
class SavedViewListView(APIView):
permission_classes = (IsAuthenticated, HasOrgContext)
@extend_schema(
tags=["Saved views"],
operation_id="saved_views_list",
parameters=[
OpenApiParameter(
"module", str, enum=sorted(LISTS), description="Only this list's views."
)
],
responses=OpenApiTypes.OBJECT,
)
def get(self, request):
profile = request.profile
views = SavedView.objects.filter(org=profile.org, profile=profile)
module = request.query_params.get("module")
if module is not None:
if module not in LISTS:
return Response(
{"module": [f"Choose one of: {', '.join(sorted(LISTS))}."]},
status=status.HTTP_400_BAD_REQUEST,
)
views = views.filter(module=module)
views = views.order_by("module", Lower("name"))
return Response(
{
"saved_views": SavedViewSerializer(views, many=True).data,
"limit": MAX_VIEWS_PER_LIST,
}
)
@extend_schema(
tags=["Saved views"],
request=SavedViewSerializer,
responses={201: SavedViewSerializer},
)
def post(self, request):
serializer = SavedViewSerializer(
data=request.data, context={"request": request}
)
serializer.is_valid(raise_exception=True)
view = serializer.save(org=request.profile.org, profile=request.profile)
return Response(SavedViewSerializer(view).data, status=status.HTTP_201_CREATED)
class SavedViewDetailView(APIView):
permission_classes = (IsAuthenticated, HasOrgContext)
def get_view(self, request, pk):
return get_scoped_or_404(
SavedView, pk, request.profile.org, profile=request.profile
)
@extend_schema(tags=["Saved views"], responses=SavedViewSerializer)
def get(self, request, pk):
return Response(SavedViewSerializer(self.get_view(request, pk)).data)
@extend_schema(
tags=["Saved views"],
request=SavedViewSerializer,
responses=SavedViewSerializer,
)
def patch(self, request, pk):
serializer = SavedViewSerializer(
self.get_view(request, pk),
data=request.data,
partial=True,
context={"request": request},
)
serializer.is_valid(raise_exception=True)
serializer.save()
return Response(serializer.data)
@extend_schema(tags=["Saved views"], responses={204: None})
def delete(self, request, pk):
self.get_view(request, pk).delete()
return Response(status=status.HTTP_204_NO_CONTENT)

View file

@ -1,4 +1,5 @@
import json
import uuid
from django.shortcuts import get_object_or_404
from drf_spectacular.utils import extend_schema, inline_serializer
@ -17,7 +18,7 @@ from common.serializer import (
APISettingsSwaggerSerializer,
ProfileSerializer,
)
from common.utils import get_or_create_tags, handle_m2m_assignment
from common.utils import get_or_create_tags
from common.validators import payload_id_list
@ -57,15 +58,42 @@ def _tag_names(value):
return names
def _parse_relations(params):
"""(tag names, profile ids) from the body, or a 400 response.
def _active_members(value, org):
"""The org's active profiles named by `value`, or a DRF 400.
Parsed before anything is saved, so a malformed value leaves no
half-written setting behind.
Every id has to be an active profile in `org`. Another org's profile, an
inactive one, or an id that matches nothing is refused rather than quietly
dropped: `webforms/legacy.py` hands leads to these profiles, so the caller
has to learn that an assignee they named was not attached.
"""
ids = payload_id_list(value, "lead_assigned_to")
wanted = {uuid.UUID(str(pk)) for pk in ids}
profiles = list(Profile.objects.filter(id__in=wanted, org=org, is_active=True))
if len(profiles) != len(wanted):
raise DRFValidationError(
{
"lead_assigned_to": [
"Each assignee must be an active member of this organization."
]
}
)
return profiles
def _parse_relations(params, org, partial=False):
"""(tag names, assignee profiles, error response) from the body.
Resolved before anything is saved, so a bad value leaves no half-written
setting behind. A key absent from a PATCH (`partial`) comes back as
`None`, meaning "leave it as it is"; on POST and PUT an absent key is an
empty list, since those replace the whole setting.
"""
try:
names = _tag_names(params.get("tags"))
ids = payload_id_list(params.get("lead_assigned_to"), "lead_assigned_to")
names = profiles = None
if not partial or "tags" in params:
names = _tag_names(params.get("tags"))
if not partial or "lead_assigned_to" in params:
profiles = _active_members(params.get("lead_assigned_to"), org)
except DRFValidationError as exc:
return (
None,
@ -75,26 +103,20 @@ def _parse_relations(params):
status=status.HTTP_400_BAD_REQUEST,
),
)
return names, ids, None
return names, profiles, None
def _attach_relations(setting, names, ids, org):
"""Attach tags and assignees, both resolved only inside `org`.
def _set_relations(setting, names, profiles, org):
"""Replace tags and assignees; `None` leaves that relation untouched.
Tags are found or created by `Tags.slug_for` within the org. Assignees are
the org's active profiles among `ids`; any other id is ignored, as on every
other M2M write. `webforms/legacy.py` hands leads to these profiles, so a
profile from another org here would receive that org's leads.
Tags are found or created by `Tags.slug_for` within `org` only, so a name
another org also uses never reaches that org's tag. `profiles` has already
been confined to the org's active members by `_active_members`.
"""
setting.tags.add(*get_or_create_tags(names, org))
handle_m2m_assignment(
setting,
"lead_assigned_to",
ids,
Profile,
org,
extra_filters={"is_active": True},
)
if names is not None:
setting.tags.set(get_or_create_tags(names, org))
if profiles is not None:
setting.lead_assigned_to.set(profiles)
class DomainList(APIView):
@ -157,17 +179,14 @@ class DomainList(APIView):
if not is_org_admin(request.profile):
return _admin_required()
params = request.data
tag_names, assignee_ids, error = _parse_relations(params)
org = request.profile.org
tag_names, assignees, error = _parse_relations(params, org)
if error:
return error
serializer = APISettingsSerializer(data=params)
if serializer.is_valid():
settings_obj = serializer.save(
created_by=request.profile.user, org=request.profile.org
)
_attach_relations(
settings_obj, tag_names, assignee_ids, request.profile.org
)
settings_obj = serializer.save(created_by=request.profile.user, org=org)
_set_relations(settings_obj, tag_names, assignees, org)
return Response(
{"error": False, "message": "API key added sucessfully"},
status=status.HTTP_201_CREATED,
@ -231,15 +250,14 @@ class DomainDetailView(APIView):
return _admin_required()
api_setting = self.get_object(pk)
params = request.data
tag_names, assignee_ids, error = _parse_relations(params)
org = request.profile.org
tag_names, assignees, error = _parse_relations(params, org)
if error:
return error
serializer = APISettingsSerializer(data=params, instance=api_setting)
if serializer.is_valid():
api_setting = serializer.save()
api_setting.tags.clear()
api_setting.lead_assigned_to.clear()
_attach_relations(api_setting, tag_names, assignee_ids, request.profile.org)
_set_relations(api_setting, tag_names, assignees, org)
return Response(
{"error": False, "message": "API setting Updated sucessfully"},
status=status.HTTP_200_OK,
@ -265,16 +283,22 @@ class DomainDetailView(APIView):
},
)
def patch(self, request, pk, **kwargs):
"""Handle partial updates to API settings."""
"""Partial update: `tags` and `lead_assigned_to` are replaced when sent
and left alone when absent, with the same org scoping as PUT."""
if not is_org_admin(request.profile):
return _admin_required()
api_setting = self.get_object(pk)
params = request.data
org = request.profile.org
tag_names, assignees, error = _parse_relations(params, org, partial=True)
if error:
return error
serializer = APISettingsSerializer(
data=params, instance=api_setting, partial=True
)
if serializer.is_valid():
api_setting = serializer.save()
_set_relations(api_setting, tag_names, assignees, org)
return Response(
{"error": False, "message": "API setting Updated successfully"},
status=status.HTTP_200_OK,

View file

@ -184,7 +184,7 @@ class TagsListView(APIView, LimitOffsetPagination):
def post(self, request, *args, **kwargs):
"""Create a new tag (admin only)."""
# Admin only for create
if not is_org_admin(request.profile) and not request.user.is_superuser:
if not is_org_admin(request.profile):
return Response(
{"error": True, "errors": "Only admins can create tags"},
status=status.HTTP_403_FORBIDDEN,
@ -313,7 +313,7 @@ class TagsDetailView(APIView):
def put(self, request, pk, *args, **kwargs):
"""Update a tag (admin only)."""
# Admin only
if not is_org_admin(request.profile) and not request.user.is_superuser:
if not is_org_admin(request.profile):
return Response(
{"error": True, "errors": "Only admins can update tags"},
status=status.HTTP_403_FORBIDDEN,
@ -404,7 +404,7 @@ class TagsDetailView(APIView):
def delete(self, request, pk, **kwargs):
"""Archive a tag - soft delete (admin only)."""
# Admin only
if not is_org_admin(request.profile) and not request.user.is_superuser:
if not is_org_admin(request.profile):
return Response(
{"error": True, "errors": "Only admins can archive tags"},
status=status.HTTP_403_FORBIDDEN,
@ -452,7 +452,7 @@ class TagsRestoreView(APIView):
def post(self, request, pk, **kwargs):
"""Restore an archived tag (admin only)."""
# Admin only
if not is_org_admin(request.profile) and not request.user.is_superuser:
if not is_org_admin(request.profile):
return Response(
{"error": True, "errors": "Only admins can restore tags"},
status=status.HTTP_403_FORBIDDEN,
@ -519,7 +519,7 @@ class TagsMergeView(APIView):
)
def post(self, request, pk, **kwargs):
"""Merge the tag at `pk` into the tag named by `into` (admin only)."""
if not is_org_admin(request.profile) and not request.user.is_superuser:
if not is_org_admin(request.profile):
return Response(
{"error": True, "errors": "Only admins can merge tags"},
status=status.HTTP_403_FORBIDDEN,

View file

@ -97,10 +97,7 @@ class UsersListView(APIView, LimitOffsetPagination):
},
)
def post(self, request, format=None):
if (
not is_org_admin(self.request.profile)
and not self.request.user.is_superuser
):
if not is_org_admin(self.request.profile):
return Response(
{"error": True, "errors": "Permission Denied"},
status=status.HTTP_403_FORBIDDEN,
@ -192,10 +189,7 @@ class UsersListView(APIView, LimitOffsetPagination):
status=status.HTTP_400_BAD_REQUEST,
)
if (
not is_org_admin(self.request.profile)
and not self.request.user.is_superuser
):
if not is_org_admin(self.request.profile):
return Response(
{"error": True, "errors": "Permission Denied"},
status=status.HTTP_403_FORBIDDEN,
@ -296,7 +290,7 @@ class UserDetailView(APIView):
Editing your own profile for contact details stays allowed; only the
privileged fields are withheld, by making them read_only downstream.
"""
actor_is_admin = is_org_admin(request.profile) or request.user.is_superuser
actor_is_admin = is_org_admin(request.profile)
editing_self = request.profile.id == target_profile.id
return actor_is_admin and not editing_self
@ -375,7 +369,6 @@ class UserDetailView(APIView):
address_obj = profile.address
if (
not is_org_admin(self.request.profile)
and not self.request.user.is_superuser
and self.request.profile.id != profile.id
):
return Response(
@ -389,7 +382,10 @@ class UserDetailView(APIView):
status=status.HTTP_403_FORBIDDEN,
)
serializer = CreateUserSerializer(
data=params, instance=profile.user, org=request.profile.org
data=params,
instance=profile.user,
org=request.profile.org,
editing_self=request.profile.id == profile.id,
)
address_serializer = BillingAddressSerializer(data=params, instance=address_obj)
# Role and the access flags may be set only by an admin editing someone
@ -417,9 +413,7 @@ class UserDetailView(APIView):
)
if address_serializer.is_valid():
address_obj = address_serializer.save(org=request.profile.org)
user = serializer.save()
user.email = user.email
user.save()
serializer.save()
if profile_serializer.is_valid():
profile = profile_serializer.save()
return Response(
@ -452,7 +446,6 @@ class UserDetailView(APIView):
profile = self.get_object(pk)
if (
not is_org_admin(self.request.profile)
and not self.request.user.is_superuser
and self.request.profile.id != profile.id
):
return Response(
@ -470,7 +463,11 @@ class UserDetailView(APIView):
)
serializer = CreateUserSerializer(
data=params, instance=profile.user, org=request.profile.org, partial=True
data=params,
instance=profile.user,
org=request.profile.org,
partial=True,
editing_self=request.profile.id == profile.id,
)
profile_serializer = CreateProfileSerializer(
data=params,
@ -562,10 +559,7 @@ class UserStatusView(APIView):
},
)
def post(self, request, pk, format=None):
if (
not is_org_admin(self.request.profile)
and not self.request.user.is_superuser
):
if not is_org_admin(self.request.profile):
return Response(
{
"error": True,

View file

@ -1,18 +1,15 @@
"""Who may open a contact, and so who may link one to a record.
Extracted from ``ContactDetailView.assert_contact_access`` so the attachment
download view asks the same question rather than carrying a second copy of the
answer. The detail view still calls it.
One definition, asked by the detail view (through ``visible_contacts_qs``, in
the lookup itself, so a hidden contact is the same 404 as a missing one) and by
the attachment download and the link checks (``has_contact_access``).
"""
from django.db.models import Q
from rest_framework.exceptions import PermissionDenied
from common.permissions import is_org_admin
from contacts.models import Contact
_DENIED = "You do not have Permission to perform this action"
def contact_account_ids(contact):
"""Every account this contact is joined to, by either route."""
@ -45,7 +42,7 @@ def has_contact_access(profile, contact):
the admin branch, it does not check the org itself: every caller fetches
``contact`` with ``org=profile.org`` first.
"""
if is_org_admin(profile) or profile.user.is_superuser:
if is_org_admin(profile):
return True
if profile.user_id == contact.created_by_id:
return True
@ -58,7 +55,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) or profile.user.is_superuser:
if is_org_admin(profile):
return qs
my_accounts = profile.account_assigned_users.values_list("id", flat=True)
return qs.filter(
@ -92,9 +89,3 @@ def replace_visible_contacts(related, contact_ids, profile):
kept = related.exclude(id__in=visible.values("id"))
wanted = visible.filter(id__in=contact_ids) if contact_ids else []
related.set([*kept, *wanted])
def assert_contact_access(profile, contact):
"""Raise 403 unless ``profile`` may open ``contact``."""
if not has_contact_access(profile, contact):
raise PermissionDenied(_DENIED)

View file

@ -168,7 +168,7 @@ class TestContactDetailAccess:
def test_stranger_is_refused(self, user_client, contact):
"""The predicate has to be able to say no, too."""
response = user_client.get(_detail(contact.id))
assert response.status_code == status.HTTP_403_FORBIDDEN
assert response.status_code == status.HTTP_404_NOT_FOUND
def test_another_orgs_contact_is_not_found(self, org_b_client, contact):
response = org_b_client.get(_detail(contact.id))
@ -240,14 +240,14 @@ class TestTheVerbsAgree:
def test_put_refuses_before_it_validates(self, _email, user_client, contact):
"""A rejected payload must not double as a permission oracle.
Sending garbage as somebody with no access should say 403, not report
Sending garbage as somebody with no access should say 404, not report
which fields were wrong -- otherwise the error body tells a stranger
what the record's shape is.
"""
response = user_client.put(
_detail(contact.id), {"email": "nope"}, format="json"
)
assert response.status_code == status.HTTP_403_FORBIDDEN
assert response.status_code == status.HTTP_404_NOT_FOUND
@pytest.mark.django_db
@ -340,7 +340,7 @@ class TestCommentsAreRecorded:
response = user_client.post(
_detail(contact.id), {"comment": "hello"}, format="json"
)
assert response.status_code == status.HTTP_403_FORBIDDEN
assert response.status_code == status.HTTP_404_NOT_FOUND
assert Comment.objects.filter(object_id=contact.id).count() == 0
def test_editing_a_comment_that_is_gone_is_not_found(self, admin_client):
@ -364,13 +364,14 @@ class TestAttachmentDeleteIsScoped:
assert Attachments.objects.filter(id=their_attachment.id).exists()
def test_the_uploader_can_delete_their_own(
self, user_client, contact, regular_user, org_a
self, user_profile, user_client, contact, regular_user, org_a
):
attachment = Attachments(
file_name="mine.txt", content_object=contact, org=org_a
)
attachment.attachment = SimpleUploadedFile("mine.txt", b"mine")
attachment.save()
contact.assigned_to.add(user_profile) # can open it
_created_by(attachment, regular_user)
response = user_client.delete(_attachment(attachment.id))
@ -378,13 +379,14 @@ class TestAttachmentDeleteIsScoped:
assert not Attachments.objects.filter(id=attachment.id).exists()
def test_somebody_elses_upload_is_refused(
self, user_client, contact, admin_user, org_a
self, user_profile, user_client, contact, admin_user, org_a
):
attachment = Attachments(
file_name="theirs.txt", content_object=contact, org=org_a
)
attachment.attachment = SimpleUploadedFile("theirs.txt", b"theirs")
attachment.save()
contact.assigned_to.add(user_profile) # can open it
_created_by(attachment, admin_user)
response = user_client.delete(_attachment(attachment.id))

View file

@ -88,7 +88,7 @@ class TestQueryset:
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
assert response.status_code == status.HTTP_404_NOT_FOUND
def test_superuser_may_open_it(self, superuser, user_client, others_contact):
response = user_client.get(f"/api/contacts/{others_contact.pk}/")
@ -132,7 +132,7 @@ class TestList:
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 response.status_code == status.HTTP_404_NOT_FOUND
assert Contact.objects.filter(pk=others_contact.pk).exists()
def test_superuser_may_delete_it(self, superuser, user_client, others_contact):

View file

@ -0,0 +1,133 @@
"""A contact the caller may not open answers exactly like one that does not exist.
Owner decision for 1.11.0: the contact detail, comment and attachment
endpoints look the contact up through `contacts.access.visible_contacts_qs`
in one step. They used to fetch org-wide and then check, answering 403 for a
hidden contact and 404 for a missing one. Each test compares the two
responses WHOLE. Writes narrower than reads keep their 403, only for a caller
who can open the contact.
"""
import pytest
from django.contrib.contenttypes.models import ContentType
from common.models import Attachments, Comment
from contacts.models import Contact
MISSING = "11111111-1111-1111-1111-111111111111"
@pytest.fixture
def contact(org_a):
"""In the caller's org, created by nobody, assigned to nobody: hidden."""
return Contact.objects.create(
first_name="Hidden", last_name="Person", email="hidden@example.com", org=org_a
)
@pytest.fixture
def comment(contact, admin_profile, org_a):
return Comment.objects.create(
content_type=ContentType.objects.get_for_model(Contact),
object_id=contact.id,
comment="Not yours",
commented_by=admin_profile,
org=org_a,
)
@pytest.fixture
def attachment(contact, admin_user, org_a):
return Attachments.objects.create(
content_type=ContentType.objects.get_for_model(Contact),
object_id=contact.id,
file_name="x.txt",
attachment="attachments/x.txt",
created_by=admin_user,
org=org_a,
)
def _same(client, method, hidden_url, missing_url, body=None):
call = getattr(client, method)
kwargs = {"format": "json"} if body is not None else {}
args = (body,) if body is not None else ()
hidden = call(hidden_url, *args, **kwargs)
missing = call(missing_url, *args, **kwargs)
assert hidden.status_code == missing.status_code == 404
assert hidden.json() == missing.json()
@pytest.mark.django_db
class TestContactIdEndpoints:
@pytest.mark.parametrize(
"method,body",
[
("get", None),
("put", {"first_name": "Mine", "last_name": "Now"}),
("patch", {"first_name": "Mine"}),
("post", {"comment": "hello"}),
("delete", None),
],
)
def test_detail(self, user_client, contact, method, body):
_same(
user_client,
method,
f"/api/contacts/{contact.id}/",
f"/api/contacts/{MISSING}/",
body,
)
contact.refresh_from_db()
assert contact.first_name == "Hidden"
@pytest.mark.django_db
class TestCommentsAndFiles:
@pytest.mark.parametrize(
"method,body",
[("put", {"comment": "x"}), ("patch", {"comment": "x"}), ("delete", None)],
)
def test_comment_on_hidden_contact(self, user_client, comment, method, body):
_same(
user_client,
method,
f"/api/contacts/comment/{comment.id}/",
f"/api/contacts/comment/{MISSING}/",
body,
)
assert Comment.objects.filter(id=comment.id).exists()
def test_attachment_on_hidden_contact(self, user_client, attachment):
_same(
user_client,
"delete",
f"/api/contacts/attachment/{attachment.id}/",
f"/api/contacts/attachment/{MISSING}/",
)
assert Attachments.objects.filter(id=attachment.id).exists()
def test_readable_contact_non_author_is_403(
self, user_client, user_profile, contact, comment, attachment
):
contact.assigned_to.add(user_profile)
url = f"/api/contacts/comment/{comment.id}/"
assert user_client.put(url, {"comment": "x"}, format="json").status_code == 403
assert user_client.delete(url).status_code == 403
attachment_url = f"/api/contacts/attachment/{attachment.id}/"
assert user_client.delete(attachment_url).status_code == 403
@pytest.mark.django_db
class TestDeleteStaysNarrower:
def test_assignee_who_is_not_creator_gets_403(
self, user_client, user_profile, contact
):
contact.assigned_to.add(user_profile)
assert user_client.get(f"/api/contacts/{contact.id}/").status_code == 200
assert user_client.delete(f"/api/contacts/{contact.id}/").status_code == 403
assert Contact.objects.filter(id=contact.id).exists()
def test_creator_may_delete(self, user_client, regular_user, contact):
Contact.objects.filter(id=contact.id).update(created_by=regular_user)
assert user_client.delete(f"/api/contacts/{contact.id}/").status_code == 200

View file

@ -130,7 +130,7 @@ class TestAccountCreate:
account = self._create(user_client, people, user_profile, "mine", "hidden")
assert _names(account) == {"Mine"}
opened = user_client.get(f"/api/contacts/{people['hidden'].id}/")
assert opened.status_code == 403
assert opened.status_code == 404
def test_member_links_a_contact_they_can_open(
self, user_client, user_profile, people
@ -161,7 +161,7 @@ class TestCreate:
obj = self._post(user_client, kind, people, "mine", "hidden")
assert _names(obj) == {"Mine"}
opened = user_client.get(f"/api/contacts/{people['hidden'].id}/")
assert opened.status_code == 403
assert opened.status_code == 404
def test_member_links_a_contact_they_can_open(self, user_client, people, kind):
obj = self._post(user_client, kind, people, "mine")
@ -209,7 +209,7 @@ class TestReplace:
linked = self._send(user_client, verb, record, people, "mine", "stranger")
assert linked == {"Mine", "Hidden"}
opened = user_client.get(f"/api/contacts/{people['stranger'].id}/")
assert opened.status_code == 403
assert opened.status_code == 404
@pytest.mark.parametrize("verb", ["put", "patch"])
def test_member_links_a_contact_they_can_open(
@ -277,7 +277,7 @@ class TestLeadConversion:
assert "Hidden" not in _names(account)
assert "Mine" in _names(account)
opened = user_client.get(f"/api/contacts/{people['hidden'].id}/")
assert opened.status_code == 403
assert opened.status_code == 404
def test_a_hidden_contact_matched_by_email_is_not_linked(
self, user_client, lead, people
@ -290,7 +290,7 @@ class TestLeadConversion:
assert _names(account) == set()
assert hidden.account_id is None
opened = user_client.get(f"/api/contacts/{hidden.id}/")
assert opened.status_code == 403
assert opened.status_code == 404
def test_a_visible_contact_matched_by_email_is_linked(
self, user_client, lead, people

View file

@ -474,7 +474,7 @@ class TestContactDetailView:
created_by=admin_user,
)
response = user_client.delete(_detail_url(contact.pk))
assert response.status_code == status.HTTP_403_FORBIDDEN
assert response.status_code == status.HTTP_404_NOT_FOUND
def test_get_detail_non_admin_not_assigned_forbidden(
self, user_client, admin_user, org_a, user_profile
@ -489,16 +489,12 @@ class TestContactDetailView:
created_by=admin_user,
)
response = user_client.get(_detail_url(contact.pk))
assert response.status_code in (status.HTTP_200_OK, status.HTTP_403_FORBIDDEN)
if response.status_code == status.HTTP_200_OK:
data = response.data
if "error" in data:
assert data["error"] is True
assert response.status_code == status.HTTP_404_NOT_FOUND
def test_add_comment_non_admin_not_assigned_forbidden(
self, user_client, admin_user, org_a, user_profile
):
"""Non-admin user not assigned/creator gets 403 when adding comment."""
"""Non-admin user not assigned/creator gets 404 when adding comment."""
_set_rls(org_a)
contact = Contact.objects.create(
first_name="CommentForbid",
@ -512,7 +508,7 @@ class TestContactDetailView:
{"comment": "Should fail"},
format="json",
)
assert response.status_code == status.HTTP_403_FORBIDDEN
assert response.status_code == status.HTTP_404_NOT_FOUND
def test_patch_contact_partial_update(self, admin_client, contact_a):
"""PATCH allows partial field updates."""
@ -586,7 +582,7 @@ class TestContactDetailView:
def test_patch_contact_non_admin_not_creator_forbidden(
self, user_client, admin_user, org_a, user_profile
):
"""Non-admin not assigned/creator gets 403 on PATCH."""
"""Non-admin not assigned/creator gets 404 on PATCH, as on GET."""
_set_rls(org_a)
contact = Contact.objects.create(
first_name="ForbidPatch",
@ -600,7 +596,7 @@ class TestContactDetailView:
{"first_name": "Hacked"},
format="json",
)
assert response.status_code == status.HTTP_403_FORBIDDEN
assert response.status_code == status.HTTP_404_NOT_FOUND
def test_patch_contact_invalid_data(self, admin_client, contact_a):
"""PATCH with invalid data returns 400."""
@ -678,7 +674,8 @@ class TestContactCommentView:
self, user_client, comment_fixture, user_profile
):
"""Non-admin non-author gets 403 on PUT."""
_, comment = comment_fixture
record, comment = comment_fixture
record.assigned_to.add(user_profile) # can open the contact
response = user_client.put(
_comment_url(comment.id),
{"comment": "Hacked"},
@ -690,7 +687,8 @@ class TestContactCommentView:
self, user_client, comment_fixture, user_profile
):
"""Non-admin non-author gets 403 on DELETE."""
_, comment = comment_fixture
record, comment = comment_fixture
record.assigned_to.add(user_profile) # can open the contact
response = user_client.delete(_comment_url(comment.id))
assert response.status_code == status.HTTP_403_FORBIDDEN
@ -735,6 +733,7 @@ class TestContactAttachmentView:
org=org_a,
created_by=admin_user,
)
contact.assigned_to.add(user_profile) # can open it
ct = ContentType.objects.get_for_model(Contact)
attachment = Attachments.objects.create(
content_type=ct,
@ -1047,7 +1046,7 @@ class TestContactDetailUpdateM2M:
def test_update_contact_non_admin_not_creator_forbidden(
self, mock_email, user_client, admin_user, org_a, user_profile
):
"""Non-admin non-creator gets 403 on PUT (lines 261-266)."""
"""Non-admin non-creator gets 404 on PUT, as on GET."""
_set_rls(org_a)
contact = Contact.objects.create(
first_name="ForbidUpdate",
@ -1065,7 +1064,7 @@ class TestContactDetailUpdateM2M:
},
format="json",
)
assert response.status_code == status.HTTP_403_FORBIDDEN
assert response.status_code == status.HTTP_404_NOT_FOUND
@patch("contacts.views.send_email_to_assigned_user.delay")
def test_update_contact_invalid_data(self, mock_email, admin_client, contact_a):
@ -1207,7 +1206,7 @@ class TestContactDetailGetNonAdmin:
def test_detail_non_admin_not_assigned_not_creator_forbidden(
self, user_client, admin_user, org_a, user_profile
):
"""Non-admin non-assigned non-creator gets 403 (lines 380-388)."""
"""Non-admin non-assigned non-creator gets 404, as for a missing id."""
_set_rls(org_a)
contact = self._create_contact_with_creator(
admin_user,
@ -1217,7 +1216,7 @@ class TestContactDetailGetNonAdmin:
email="forbiddendetailc@example.com",
)
response = user_client.get(_detail_url(contact.pk))
assert response.status_code == status.HTTP_403_FORBIDDEN
assert response.status_code == status.HTTP_404_NOT_FOUND
@pytest.mark.django_db
@ -1352,7 +1351,8 @@ class TestContactCommentPatchView:
self, user_client, comment_fix, user_profile
):
"""Non-admin non-author gets 403 on PATCH comment (lines 735-739)."""
_, comment = comment_fix
record, comment = comment_fix
record.assigned_to.add(user_profile) # can open the contact
response = user_client.patch(
_comment_url(comment.id),
{"comment": "Hacked patch"},

View file

@ -68,7 +68,7 @@ class TestLinkedContactsOverTheApi:
"""A non-admin assignee of the parent, who cannot open the contact."""
def test_the_viewer_cannot_open_the_contact(self, user_client, person):
assert user_client.get(f"/api/contacts/{person.id}/").status_code == 403
assert user_client.get(f"/api/contacts/{person.id}/").status_code == 404
def test_ticket_carries_name_and_email_only(
self, user_client, org_a, user_profile, person

View file

@ -1,9 +1,7 @@
import json
from django.contrib.contenttypes.models import ContentType
from django.core.exceptions import ValidationError as DjangoValidationError
from django.db.models import Count, Q, Sum
from django.http import Http404
from django.shortcuts import get_object_or_404
from drf_spectacular.utils import (
extend_schema,
@ -17,6 +15,7 @@ from rest_framework.views import APIView
from cases.access import visible_cases_qs
from common.custom_fields import validate_payload as validate_custom_fields_payload
from common.lookups import get_on_visible_record_or_404
from common.models import (
Attachments,
Comment,
@ -307,29 +306,22 @@ class ContactDetailView(APIView):
model = Contact
def get_object(self, pk):
try:
return get_object_or_404(
Contact.objects.select_related("account").prefetch_related(
"account_contacts", "assigned_to__user", "teams", "tags"
),
pk=pk,
org=self.request.profile.org,
)
except (DjangoValidationError, ValueError):
# The route matches <str:pk>, so anything at all can arrive here.
# UUIDField.to_python raises Django's ValidationError, which
# get_object_or_404 does not catch, so /api/contacts/banana/ was a
# 500 -- an error report for a URL somebody simply mistyped.
raise Http404("No such contact.")
"""The contact, or 404 when it is missing or the caller may not open it.
def assert_contact_access(self, contact):
"""Delegates to `contacts.access`, which holds the one definition.
The attachment download view asks the same question. Four verbs once
carried four copies of this check and all four had the same dead
branch, which is the reason it lives in exactly one place now.
Looked up through the read rule (`contacts.access.visible_contacts_qs`)
in the same query, so a same-org contact the caller cannot open
answers every verb exactly as an id that does not exist. Fetching
org-wide and checking afterwards answered 403, which confirmed it.
"""
access.assert_contact_access(self.request.profile, contact)
profile = self.request.profile
return get_object_or_404(
Contact.objects.select_related("account").prefetch_related(
"account_contacts", "assigned_to__user", "teams", "tags"
),
pk=pk,
org=profile.org,
id__in=access.visible_contacts_qs(profile).values("id"),
)
@staticmethod
def account_ids(contact):
@ -449,7 +441,6 @@ class ContactDetailView(APIView):
def put(self, request, pk, format=None):
data = request.data
contact_obj = self.get_object(pk=pk)
self.assert_contact_access(contact_obj)
contact_serializer = CreateContactSerializer(
data=data, instance=contact_obj, request_obj=request
@ -546,7 +537,6 @@ class ContactDetailView(APIView):
def get(self, request, pk, format=None):
context = {}
contact_obj = self.get_object(pk)
self.assert_contact_access(contact_obj)
context["contact_obj"] = ContactSerializer(contact_obj).data
assigned_data = []
for each in contact_obj.assigned_to.all():
@ -659,13 +649,13 @@ 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, a superuser or the person who
# entered it may destroy the record. The account and deal delete
# Deliberately narrower than reading: an assignee may work on a
# contact, only an admin, a superuser or the person who entered it may
# destroy the record. The caller can open it by now, so this refusal
# is an honest 403. 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(
@ -705,7 +695,6 @@ class ContactDetailView(APIView):
# contact's name, email, phone and address back in the response. It
# also raised DoesNotExist -- a 500 -- for an id that was merely gone.
self.contact_obj = self.get_object(pk)
self.assert_contact_access(self.contact_obj)
# Before the comment is saved, so a refused file does not leave it posted.
validate_attachment(self.request.FILES.get("contact_attachment"))
if params.get("comment"):
@ -786,7 +775,6 @@ class ContactDetailView(APIView):
"""Handle partial updates to a contact."""
data = request.data
contact_obj = self.get_object(pk=pk)
self.assert_contact_access(contact_obj)
contact_serializer = CreateContactSerializer(
data=data, instance=contact_obj, request_obj=request, partial=True
@ -864,12 +852,13 @@ class ContactCommentView(APIView):
permission_classes = (IsAuthenticated, HasOrgContext)
def get_object(self, pk):
# `.get()` raised DoesNotExist for a comment somebody else had already
# deleted, which reached the client as a 500.
try:
return get_object_or_404(self.model, pk=pk, org=self.request.profile.org)
except (DjangoValidationError, ValueError):
raise Http404("No such comment.")
"""404, like a missing id, for a comment on a contact the caller cannot open."""
return get_on_visible_record_or_404(
self.model,
pk,
self.request.profile.org,
access.visible_contacts_qs(self.request.profile),
)
@extend_schema(
tags=["contacts"],
@ -1008,10 +997,15 @@ class ContactAttachmentView(APIView):
# And `request.profile == self.object.created_by` compares a Profile to
# a User, so it was always False: the person who uploaded a file could
# not delete it unless they were an admin.
try:
self.object = get_object_or_404(self.model, pk=pk, org=request.profile.org)
except (DjangoValidationError, ValueError):
raise Http404("No such attachment.")
#
# A file on a contact the caller cannot open is the same 404 as a
# missing id.
self.object = get_on_visible_record_or_404(
self.model,
pk,
request.profile.org,
access.visible_contacts_qs(request.profile),
)
if (
is_org_admin(request.profile)
or request.profile.user_id == self.object.created_by_id

View file

@ -355,6 +355,17 @@ REST_FRAMEWORK = {
# "rest_framework.authentication.SessionAuthentication",
# "rest_framework.authentication.BasicAuthentication",
),
# How many reverse proxies in front of Django append to X-Forwarded-For.
# `common.request_meta.client_ip` (every recorded IP and every per-IP
# throttle) believes only that many entries from the right. Unset means
# none: REMOTE_ADDR is used. Leave it unset when the app server already
# resolves the client from trusted proxies (uvicorn --proxy-headers
# --forwarded-allow-ips, as hosted production runs); counting the proxy
# again here would make every SvelteKit-relayed request the relay itself.
# Set it only when REMOTE_ADDR is the proxy (e.g. gunicorn behind nginx).
"NUM_PROXIES": int(os.environ["NUM_PROXIES"])
if os.environ.get("NUM_PROXIES")
else None,
"DEFAULT_PAGINATION_CLASS": "rest_framework.pagination.LimitOffsetPagination",
"PAGE_SIZE": 10,
"DEFAULT_SCHEMA_CLASS": "drf_spectacular.openapi.AutoSchema",
@ -370,7 +381,7 @@ REST_FRAMEWORK = {
# clicks through several articles and a crawler walks the sitemap.
"help_center_ip": os.environ.get("HELP_CENTER_THROTTLE_IP", "600/hour"),
# Public help center pages, per help center across all visitors. The
# per-visitor bucket trusts X-Forwarded-For; this one does not.
# backstop for a scraper spread across many addresses.
"help_center_global": os.environ.get(
"HELP_CENTER_THROTTLE_GLOBAL", "10000/hour"
),

View file

@ -1,3 +1,4 @@
from django import forms
from django.contrib import admin
from invoices.models import (
@ -12,9 +13,105 @@ from invoices.models import (
RecurringInvoice,
RecurringInvoiceLineItem,
)
from invoices.serializer import issued_lock_message
# The money on an issued invoice or estimate, read-only in the admin as it is
# in the API (`issued_lock_message`, owner decision 1.11.0). Status, dates,
# notes and payments stay editable.
INVOICE_AMOUNT_FIELDS = (
"currency",
"subtotal",
"discount_type",
"discount_value",
"discount_amount",
"tax_rate",
"tax_amount",
"shipping_amount",
"total_amount",
"amount_paid",
"amount_due",
)
ESTIMATE_AMOUNT_FIELDS = (
"currency",
"subtotal",
"discount_type",
"discount_value",
"discount_amount",
"tax_rate",
"tax_amount",
"total_amount",
)
class InvoiceLineItemInline(admin.TabularInline):
class IssuedAmountsReadOnlyMixin:
"""A document admin whose amount fields lock once it leaves Draft."""
amount_fields = ()
def get_readonly_fields(self, request, obj=None):
readonly = tuple(super().get_readonly_fields(request, obj))
if issued_lock_message(obj):
return readonly + self.amount_fields
return readonly
class IssuedLinesInlineMixin:
"""A line inline that can be read, not added, edited or deleted, under an
issued document. ``obj`` is the parent document here."""
def has_add_permission(self, request, obj=None):
return not issued_lock_message(obj) and super().has_add_permission(request, obj)
def has_change_permission(self, request, obj=None):
return not issued_lock_message(obj) and super().has_change_permission(
request, obj
)
def has_delete_permission(self, request, obj=None):
return not issued_lock_message(obj) and super().has_delete_permission(
request, obj
)
class IssuedLineAdminMixin:
"""The standalone line admin: a line of an issued document cannot be
changed or deleted, a new line cannot be put on one, and the bulk delete
action (which would skip the per-line check) is not offered."""
document_field = ""
def _issued(self, line):
return line is not None and issued_lock_message(
getattr(line, self.document_field)
)
def has_change_permission(self, request, obj=None):
return not self._issued(obj) and super().has_change_permission(request, obj)
def has_delete_permission(self, request, obj=None):
return not self._issued(obj) and super().has_delete_permission(request, obj)
def get_actions(self, request):
actions = super().get_actions(request)
actions.pop("delete_selected", None)
return actions
def get_form(self, request, obj=None, **kwargs):
base = super().get_form(request, obj, **kwargs)
document_field = self.document_field
class Form(base):
def clean(self):
cleaned = super().clean()
message = issued_lock_message(cleaned.get(document_field))
if message:
raise forms.ValidationError(message)
return cleaned
return Form
class InvoiceLineItemInline(IssuedLinesInlineMixin, admin.TabularInline):
model = InvoiceLineItem
extra = 1
fields = (
@ -38,7 +135,8 @@ class PaymentInline(admin.TabularInline):
@admin.register(Invoice)
class InvoiceAdmin(admin.ModelAdmin):
class InvoiceAdmin(IssuedAmountsReadOnlyMixin, admin.ModelAdmin):
amount_fields = INVOICE_AMOUNT_FIELDS
list_display = (
"invoice_number",
"invoice_title",
@ -234,7 +332,8 @@ class ProductAdmin(admin.ModelAdmin):
@admin.register(InvoiceLineItem)
class InvoiceLineItemAdmin(admin.ModelAdmin):
class InvoiceLineItemAdmin(IssuedLineAdminMixin, admin.ModelAdmin):
document_field = "invoice"
list_display = (
"invoice",
"description",
@ -285,7 +384,7 @@ class InvoiceTemplateAdmin(admin.ModelAdmin):
# Estimate Admin
class EstimateLineItemInline(admin.TabularInline):
class EstimateLineItemInline(IssuedLinesInlineMixin, admin.TabularInline):
model = EstimateLineItem
extra = 1
fields = (
@ -303,7 +402,8 @@ class EstimateLineItemInline(admin.TabularInline):
@admin.register(Estimate)
class EstimateAdmin(admin.ModelAdmin):
class EstimateAdmin(IssuedAmountsReadOnlyMixin, admin.ModelAdmin):
amount_fields = ESTIMATE_AMOUNT_FIELDS
list_display = (
"estimate_number",
"title",
@ -337,7 +437,8 @@ class EstimateAdmin(admin.ModelAdmin):
@admin.register(EstimateLineItem)
class EstimateLineItemAdmin(admin.ModelAdmin):
class EstimateLineItemAdmin(IssuedLineAdminMixin, admin.ModelAdmin):
document_field = "estimate"
list_display = (
"estimate",
"description",

View file

@ -11,6 +11,7 @@ from django.db import transaction
from django.db.models import Count, Q, Sum
from django.http import HttpResponse
from django.utils import timezone
from django.utils.decorators import method_decorator
from drf_spectacular.utils import extend_schema
from rest_framework import status
from rest_framework.exceptions import ValidationError
@ -20,6 +21,7 @@ from rest_framework.response import Response
from rest_framework.views import APIView
from common.custom_fields import validate_payload as validate_custom_fields_payload
from common.lookups import get_on_visible_record_or_404
from common.models import Attachments, Comment, CustomFieldDefinition
from common.money import currency_block, group_by_currency
from common.permissions import HasOrgContext, is_org_admin
@ -39,7 +41,6 @@ from invoices.models import (
InvoiceTemplate,
Payment,
Product,
RecurringInvoice,
)
from invoices.pdf import (
generate_estimate_filename,
@ -51,7 +52,9 @@ from invoices.permissions import (
get_estimate_or_error,
get_invoice_or_error,
get_recurring_or_error,
visible_estimates_qs,
visible_invoices_qs,
visible_recurring_qs,
)
from invoices.serializer import (
EstimateCreateSerializer,
@ -73,6 +76,7 @@ from invoices.serializer import (
RecurringInvoiceCreateSerializer,
RecurringInvoiceListSerializer,
RecurringInvoiceSerializer,
issued_lock_message,
validate_document_account,
)
from invoices.tasks import create_invoice_history, send_email, send_invoice_to_client
@ -346,9 +350,27 @@ class InvoiceDetailView(APIView):
@extend_schema(tags=["Invoices"], operation_id="invoices_update")
def put(self, request, pk, format=None):
invoice, error = get_invoice_or_error(request, pk)
with transaction.atomic():
response, invoice = self._update(request, pk)
if invoice is not None:
# After the commit, so the history task reads the saved row.
create_invoice_history.delay(
str(invoice.id),
str(request.profile.id),
[],
str(request.profile.org.id),
)
return response
def _update(self, request, pk):
"""``(response, saved_invoice_or_None)``, run inside the transaction."""
# Locked from the read to the save: the issued-document check sees the
# status this save writes back, and a send cannot land in between.
invoice, error = get_invoice_or_error(
request, pk, queryset=Invoice.objects.select_for_update()
)
if error:
return error
return error, None
save_kwargs = {}
if "custom_fields" in request.data:
@ -368,7 +390,7 @@ class InvoiceDetailView(APIView):
return Response(
{"error": True, "errors": {"custom_fields": cf_errors}},
status=status.HTTP_400_BAD_REQUEST,
)
), None
save_kwargs["custom_fields"] = cleaned_cf
serializer = InvoiceCreateSerializer(
@ -379,29 +401,20 @@ class InvoiceDetailView(APIView):
partial=True,
)
if serializer.is_valid():
invoice = serializer.save(**save_kwargs)
# Create history entry
create_invoice_history.delay(
str(invoice.id),
str(request.profile.id),
[],
str(request.profile.org.id),
)
if not serializer.is_valid():
return Response(
{
"error": False,
"message": "Invoice updated successfully",
"invoice": InvoiceSerializer(invoice).data,
}
)
{"error": True, "errors": serializer.errors},
status=status.HTTP_400_BAD_REQUEST,
), None
invoice = serializer.save(**save_kwargs)
return Response(
{"error": True, "errors": serializer.errors},
status=status.HTTP_400_BAD_REQUEST,
)
{
"error": False,
"message": "Invoice updated successfully",
"invoice": InvoiceSerializer(invoice).data,
}
), invoice
class InvoiceSendView(APIView):
@ -411,27 +424,32 @@ class InvoiceSendView(APIView):
@extend_schema(tags=["Invoices"], operation_id="invoices_send")
def post(self, request, pk):
invoice, error = get_invoice_or_error(request, pk)
if error:
return error
# A settled invoice must not be re-sent: doing so would overwrite
# sent_at/is_email_sent and mail the client about a closed invoice.
if invoice.status in ("Paid", "Cancelled"):
return Response(
{
"error": True,
"message": f"Cannot send a {invoice.status.lower()} invoice",
},
status=status.HTTP_400_BAD_REQUEST,
with transaction.atomic():
# Locked, so this full save cannot write back a copy read before
# an edit or payment committed.
invoice, error = get_invoice_or_error(
request, pk, queryset=Invoice.objects.select_for_update()
)
if error:
return error
# Update status and sent_at
if invoice.status == "Draft":
invoice.status = "Sent"
invoice.sent_at = timezone.now()
invoice.is_email_sent = True
invoice.save()
# A settled invoice must not be re-sent: doing so would overwrite
# sent_at/is_email_sent and mail the client about a closed invoice.
if invoice.status in ("Paid", "Cancelled"):
return Response(
{
"error": True,
"message": f"Cannot send a {invoice.status.lower()} invoice",
},
status=status.HTTP_400_BAD_REQUEST,
)
# Update status and sent_at
if invoice.status == "Draft":
invoice.status = "Sent"
invoice.sent_at = timezone.now()
invoice.is_email_sent = True
invoice.save()
# Send invoice email to client
send_invoice_to_client.delay(
@ -640,6 +658,20 @@ class InvoicePDFView(APIView):
# =============================================================================
def _issued_lock_response(invoice):
"""The 400 for a line change on an issued invoice, or None on a Draft.
Checked after `get_invoice_or_error`, so a caller who cannot open the
invoice learns nothing about its status.
"""
message = issued_lock_message(invoice)
if message is None:
return None
return Response(
{"error": True, "message": message}, status=status.HTTP_400_BAD_REQUEST
)
class InvoiceLineItemListView(APIView):
"""Manage line items for an invoice"""
@ -655,10 +687,18 @@ class InvoiceLineItemListView(APIView):
return Response(InvoiceLineItemSerializer(line_items, many=True).data)
@extend_schema(tags=["Invoice Line Items"], operation_id="line_items_create")
@method_decorator(transaction.atomic)
def post(self, request, invoice_id):
invoice, error = get_invoice_or_error(request, invoice_id)
# Locked from the read to the save: the issued-document check sees the
# status this save writes back, and a send cannot land in between.
invoice, error = get_invoice_or_error(
request, invoice_id, queryset=Invoice.objects.select_for_update()
)
if error:
return error
locked = _issued_lock_response(invoice)
if locked:
return locked
serializer = InvoiceLineItemCreateSerializer(
data=request.data, request_obj=request
@ -696,10 +736,18 @@ class InvoiceLineItemDetailView(APIView):
).first()
@extend_schema(tags=["Invoice Line Items"], operation_id="line_items_update")
@method_decorator(transaction.atomic)
def put(self, request, invoice_id, pk):
_, error = get_invoice_or_error(request, invoice_id)
# Locked from the read to the save: the issued-document check sees the
# status this save writes back, and a send cannot land in between.
invoice, error = get_invoice_or_error(
request, invoice_id, queryset=Invoice.objects.select_for_update()
)
if error:
return error
locked = _issued_lock_response(invoice)
if locked:
return locked
line_item = self.get_object(invoice_id, pk)
if not line_item:
@ -732,10 +780,18 @@ class InvoiceLineItemDetailView(APIView):
)
@extend_schema(tags=["Invoice Line Items"], operation_id="line_items_destroy")
@method_decorator(transaction.atomic)
def delete(self, request, invoice_id, pk):
_, error = get_invoice_or_error(request, invoice_id)
# Locked from the read to the save: the issued-document check sees the
# status this save writes back, and a send cannot land in between.
invoice, error = get_invoice_or_error(
request, invoice_id, queryset=Invoice.objects.select_for_update()
)
if error:
return error
locked = _issued_lock_response(invoice)
if locked:
return locked
line_item = self.get_object(invoice_id, pk)
if not line_item:
@ -744,7 +800,6 @@ class InvoiceLineItemDetailView(APIView):
status=status.HTTP_404_NOT_FOUND,
)
invoice = line_item.invoice
line_item.delete()
# Recalculate invoice totals
@ -891,7 +946,7 @@ class ProductListView(APIView, LimitOffsetPagination):
# need it to build line items), but changing it is an admin act, the
# same posture Organization and Team settings take. A non-admin who
# curls this endpoint is refused here, not just hidden from in the UI.
if not is_org_admin(request.profile) and not request.user.is_superuser:
if not is_org_admin(request.profile):
return Response(
{
"error": True,
@ -940,7 +995,7 @@ class ProductDetailView(APIView):
def _require_admin(self, request):
"""Editing the shared catalog is admin-only; see ProductListView.post."""
if not is_org_admin(request.profile) and not request.user.is_superuser:
if not is_org_admin(request.profile):
return Response(
{
"error": True,
@ -1012,26 +1067,13 @@ class EstimateListView(APIView, LimitOffsetPagination):
permission_classes = (IsAuthenticated, HasOrgContext)
def get_queryset(self):
org = self.request.profile.org
queryset = Estimate.objects.filter(org=org).select_related(
"""Every estimate the caller may open, before the query string."""
return visible_estimates_qs(
self.request.profile, self.request.user
).select_related(
"account", "contact", "opportunity", "created_by", "converted_to_invoice"
)
if (
not is_org_admin(self.request.profile)
and not self.request.user.is_superuser
):
# created_by is a User FK; comparing it to a Profile does not just
# silently fail here, it reaches the DB as Q(created_by=<Profile>)
# and raised ValueError -- a 500 on every non-admin estimate list.
queryset = queryset.filter(
Q(created_by=self.request.profile.user)
| Q(assigned_to=self.request.profile)
).distinct()
return queryset
@extend_schema(tags=["Estimates"], operation_id="estimates_list")
def get(self, request):
queryset = self.get_queryset()
@ -1040,8 +1082,11 @@ class EstimateListView(APIView, LimitOffsetPagination):
params = request.query_params
if params.get("status"):
queryset = queryset.filter(status=params.get("status"))
if params.get("account"):
queryset = queryset.filter(account_id=params.get("account"))
# Parsed first, as the invoice list does: raw text reached the UUID
# lookup and a malformed id answered 500.
account_id = uuid_param(params, "account")
if account_id:
queryset = queryset.filter(account_id=account_id)
if params.get("search"):
search = params.get("search")
queryset = queryset.filter(
@ -1133,8 +1178,13 @@ class EstimateDetailView(APIView):
)
@extend_schema(tags=["Estimates"], operation_id="estimates_update")
@method_decorator(transaction.atomic)
def put(self, request, pk):
estimate, error = get_estimate_or_error(request, pk)
# Locked from the read to the save: the issued-document check sees the
# status this save writes back, and a send cannot land in between.
estimate, error = get_estimate_or_error(
request, pk, queryset=Estimate.objects.select_for_update()
)
if error:
return error
@ -1200,8 +1250,13 @@ class EstimateConvertView(APIView):
permission_classes = (IsAuthenticated, HasOrgContext)
@extend_schema(tags=["Estimates"], operation_id="estimates_convert")
@method_decorator(transaction.atomic)
def post(self, request, pk):
estimate, error = get_estimate_or_error(request, pk)
# Locked, so two converts at once cannot both pass the check below and
# raise two invoices, and the save cannot write back a stale copy.
estimate, error = get_estimate_or_error(
request, pk, queryset=Estimate.objects.select_for_update()
)
if error:
return error
@ -1281,7 +1336,25 @@ class EstimateSendView(APIView):
@extend_schema(tags=["Estimates"], operation_id="estimates_send")
def post(self, request, pk):
estimate, error = get_estimate_or_error(request, pk)
with transaction.atomic():
refused = self._mark_sent(request, pk)
if refused is not None:
return refused
# Trigger async email task, after the commit so it reads the saved row
from invoices.tasks import send_estimate_to_client
send_estimate_to_client.delay(str(pk), str(request.profile.org.id))
return Response({"error": False, "message": "Estimate sent successfully"})
def _mark_sent(self, request, pk):
"""Mark it sent and return None, or return the refusal. Runs inside the
transaction, on a locked row, so this full save cannot write back a
copy read before an edit committed."""
estimate, error = get_estimate_or_error(
request, pk, queryset=Estimate.objects.select_for_update()
)
if error:
return error
@ -1323,13 +1396,7 @@ class EstimateSendView(APIView):
estimate.status = "Sent"
estimate.sent_at = timezone.now()
estimate.save()
# Trigger async email task
from invoices.tasks import send_estimate_to_client
send_estimate_to_client.delay(str(estimate.id), str(request.profile.org.id))
return Response({"error": False, "message": "Estimate sent successfully"})
return None
class EstimatePDFView(APIView):
@ -1380,21 +1447,14 @@ class RecurringInvoiceListView(APIView, LimitOffsetPagination):
@extend_schema(tags=["Recurring Invoices"], operation_id="recurring_list")
def get(self, request):
# Non-admins see only schedules they created or are assigned to, the
# same rule as the invoice and estimate lists.
queryset = (
RecurringInvoice.objects.filter(org=request.profile.org)
visible_recurring_qs(request.profile, request.user)
.select_related("account", "contact")
.order_by("-created_at")
)
# Non-admins see only schedules they created or are assigned to -- the
# same scoping as the invoice and estimate lists. created_by is a User
# FK, so it is matched against request.profile.user; assigned_to holds
# Profiles, matched against request.profile.
if not is_org_admin(request.profile) and not request.user.is_superuser:
queryset = queryset.filter(
Q(created_by=request.profile.user) | Q(assigned_to=request.profile)
).distinct()
# Apply filters
params = request.query_params
if params.get("is_active"):
@ -1589,7 +1649,7 @@ def _forbid_non_admin_template(
reason, so it is the same gate rather than a second copy of the role check
that could drift away from this one.
"""
if not is_org_admin(request.profile) and not request.user.is_superuser:
if not is_org_admin(request.profile):
return Response(
{"error": True, "message": message},
status=status.HTTP_403_FORBIDDEN,
@ -1802,32 +1862,50 @@ class InvoiceCommentView(APIView):
class InvoiceCommentDetailView(APIView):
"""Update or delete a comment"""
"""Update or delete a comment on an invoice.
The comment is found through its invoice: only a comment on an invoice,
in the caller's org, on an invoice the caller may open. Anything else (a
missing id, another module's comment, a comment on an invoice this
caller cannot open) is the same 404, so the answer says nothing about
which ids exist. Among callers who can read the invoice, only the
comment's author or an admin may change it (403).
"""
permission_classes = (IsAuthenticated, HasOrgContext)
def get_object(self, pk):
return Comment.objects.filter(id=pk, org=self.request.profile.org).first()
@extend_schema(tags=["Invoice Comments"], operation_id="comments_update")
def put(self, request, pk):
comment = self.get_object(pk)
if not comment:
return Response(
{"error": True, "message": "Comment not found"},
status=status.HTTP_404_NOT_FOUND,
)
# Only comment author or admin can edit
if comment.commented_by != request.profile and not is_org_admin(
def _resolve(self, request, pk):
"""``(comment, None)``, or ``(None, response)`` with the 403. A comment
the caller may not reach raises the same 404 as a missing id."""
comment = get_on_visible_record_or_404(
Comment,
pk,
request.profile.org,
visible_invoices_qs(request.profile, request.user),
)
if comment.commented_by_id != request.profile.id and not is_org_admin(
request.profile
):
return Response(
return None, Response(
{"error": True, "message": "Permission denied"},
status=status.HTTP_403_FORBIDDEN,
)
return comment, None
comment.comment = request.data.get("comment", comment.comment)
@extend_schema(tags=["Invoice Comments"], operation_id="comments_update")
def put(self, request, pk):
comment, error = self._resolve(request, pk)
if error:
return error
# The create path refuses an empty comment; an edit may not make one.
text = request.data.get("comment", comment.comment)
if not text:
return Response(
{"error": True, "message": "Comment text required"},
status=status.HTTP_400_BAD_REQUEST,
)
comment.comment = text
comment.save()
return Response(
@ -1840,20 +1918,9 @@ class InvoiceCommentDetailView(APIView):
@extend_schema(tags=["Invoice Comments"], operation_id="comments_destroy")
def delete(self, request, pk):
comment = self.get_object(pk)
if not comment:
return Response(
{"error": True, "message": "Comment not found"},
status=status.HTTP_404_NOT_FOUND,
)
if comment.commented_by != request.profile and not is_org_admin(
request.profile
):
return Response(
{"error": True, "message": "Permission denied"},
status=status.HTTP_403_FORBIDDEN,
)
comment, error = self._resolve(request, pk)
if error:
return error
comment.delete()
return Response(
@ -1894,18 +1961,26 @@ class InvoiceAttachmentView(APIView):
class InvoiceAttachmentDetailView(APIView):
"""Delete an attachment"""
"""Delete an attachment on an invoice.
Found through its invoice, as the comment view above is: an attachment on
another module's record, in another org, or on an invoice this caller
cannot open raises the same 404 as a missing id. It used to be looked up
org-wide across every module, so an uploader who had lost access to a
lead could still delete that lead's file through this route, and anyone
else got a 403 confirming the id existed.
"""
permission_classes = (IsAuthenticated, HasOrgContext)
@extend_schema(tags=["Invoice Attachments"], operation_id="attachments_destroy")
def delete(self, request, pk):
attachment = Attachments.objects.filter(id=pk, org=request.profile.org).first()
if not attachment:
return Response(
{"error": True, "message": "Attachment not found"},
status=status.HTTP_404_NOT_FOUND,
)
attachment = get_on_visible_record_or_404(
Attachments,
pk,
request.profile.org,
visible_invoices_qs(request.profile, request.user),
)
# created_by is a User FK, so it must be compared against request.user --
# comparing it to request.profile is always unequal and locks the
@ -1942,7 +2017,7 @@ def _forbid_non_admin_reports(request):
admin-only (Django superusers included), the same bar as the org and team
settings.
"""
if not is_org_admin(request.profile) and not request.user.is_superuser:
if not is_org_admin(request.profile):
return Response(
{
"error": True,
@ -2485,7 +2560,7 @@ class InvoiceFromTimeEntriesView(APIView):
build_invoice_lines_from_entries,
)
if not is_org_admin(request.profile) and not request.user.is_superuser:
if not is_org_admin(request.profile):
return Response(
{
"error": True,

View file

@ -0,0 +1,91 @@
# Generated by Django 6.1.1 on 2026-09-26 14:51
from django.db import migrations, models
import invoices.models
class Migration(migrations.Migration):
dependencies = [
("invoices", "0011_document_number_unique_per_org"),
]
operations = [
migrations.AlterField(
model_name="estimate",
name="tax_rate",
field=models.DecimalField(
decimal_places=2,
default=0,
max_digits=5,
validators=[invoices.models.validate_tax_rate],
verbose_name="Tax Rate (%)",
),
),
migrations.AlterField(
model_name="estimatelineitem",
name="tax_rate",
field=models.DecimalField(
decimal_places=2,
default=0,
max_digits=5,
validators=[invoices.models.validate_tax_rate],
verbose_name="Tax Rate (%)",
),
),
migrations.AlterField(
model_name="invoice",
name="shipping_amount",
field=models.DecimalField(
decimal_places=2,
default=0,
max_digits=12,
validators=[invoices.models.validate_shipping_amount],
verbose_name="Shipping",
),
),
migrations.AlterField(
model_name="invoice",
name="tax_rate",
field=models.DecimalField(
decimal_places=2,
default=0,
max_digits=5,
validators=[invoices.models.validate_tax_rate],
verbose_name="Tax Rate (%)",
),
),
migrations.AlterField(
model_name="invoicelineitem",
name="tax_rate",
field=models.DecimalField(
decimal_places=2,
default=0,
max_digits=5,
validators=[invoices.models.validate_tax_rate],
verbose_name="Tax Rate (%)",
),
),
migrations.AlterField(
model_name="recurringinvoice",
name="tax_rate",
field=models.DecimalField(
decimal_places=2,
default=0,
max_digits=5,
validators=[invoices.models.validate_tax_rate],
verbose_name="Tax Rate (%)",
),
),
migrations.AlterField(
model_name="recurringinvoicelineitem",
name="tax_rate",
field=models.DecimalField(
decimal_places=2,
default=0,
max_digits=5,
validators=[invoices.models.validate_tax_rate],
verbose_name="Tax Rate (%)",
),
),
]

Some files were not shown because too many files have changed in this diff Show more