From afbb5149ce42abfb4dbeaae5a13491f82eda824c Mon Sep 17 00:00:00 2001 From: Michael Bunsen Date: Thu, 17 Sep 2026 14:01:31 -0700 Subject: [PATCH 01/15] feat(taxa): give TaxaList an explicit public/project-scoped distinction Adds TaxaList.is_public and a manage_public_taxalist permission so a taxa list can be marked available to every project instead of relying on "has zero projects" as an implicit signal. BaseQuerySet.for_project() filters a model's M2M projects field via an Exists subquery (no join, no duplicate rows) with an is_public bypass; visible_for_user() gets the same bypass so a public list is visible to every user, draft projects included. get_or_create_for_project() takes an is_public flag to look up/create the public list of a name instead of only a hidden zero-project one. The 0096 migration backfills is_public=True for every existing zero-project list, except a per-algorithm "Taxa returned by ..." category-map list, which stays hidden. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01TngW6AshkUEhNp9D6z3U9Z --- ami/base/models.py | 32 +++++++++- .../migrations/0096_taxalist_is_public.py | 58 +++++++++++++++++++ ami/main/models.py | 33 +++++++---- 3 files changed, 112 insertions(+), 11 deletions(-) create mode 100644 ami/main/migrations/0096_taxalist_is_public.py diff --git a/ami/base/models.py b/ami/base/models.py index 2f245b745..41e7bf181 100644 --- a/ami/base/models.py +++ b/ami/base/models.py @@ -1,6 +1,7 @@ from django.contrib.auth.models import AbstractUser, AnonymousUser +from django.core.exceptions import FieldDoesNotExist from django.db import models -from django.db.models import Q, QuerySet +from django.db.models import Exists, OuterRef, Q, QuerySet from guardian.shortcuts import get_perms import ami.tasks @@ -85,8 +86,37 @@ def visible_for_user(self, user: User | AnonymousUser) -> QuerySet: if not is_anonymous: filter_condition |= Q(**{f"{project_field}owner": user}) | Q(**{f"{project_field}members": user}) + # Public rows (e.g. public TaxaLists) are visible to everyone, draft or not. + if hasattr(model, "is_public"): + filter_condition |= Q(is_public=True) + return self.filter(filter_condition).distinct() + def for_project(self, project: models.Model, include_public: bool = True) -> QuerySet: + """ + Filter to rows in the model's M2M ``projects`` field for the given project, + plus every public row when the model defines ``is_public`` and ``include_public`` + is set. + + Membership is checked with an ``Exists`` subquery against the M2M through table + instead of filtering on ``projects=project`` directly, so a row linked to the + project through multiple paths cannot appear twice and no ``.distinct()`` is + needed downstream. + """ + model = self.model + try: + field = model._meta.get_field("projects") + except FieldDoesNotExist: + field = None + if not isinstance(field, models.ManyToManyField): + raise TypeError(f"{model.__name__} has no ManyToMany 'projects' field; for_project() is not applicable.") + + condition = Q(Exists(model._default_manager.filter(pk=OuterRef("pk"), projects=project))) + if include_public and hasattr(model, "is_public"): + condition |= Q(is_public=True) + + return self.filter(condition) + class BaseModel(models.Model): """ """ diff --git a/ami/main/migrations/0096_taxalist_is_public.py b/ami/main/migrations/0096_taxalist_is_public.py new file mode 100644 index 000000000..f97b6eaf1 --- /dev/null +++ b/ami/main/migrations/0096_taxalist_is_public.py @@ -0,0 +1,58 @@ +""" +Add ``TaxaList.is_public`` and backfill it for existing rows. + +A TaxaList with no project was previously treated as an ad hoc "global" list by +convention (see ``get_or_create_for_project``); this migration makes that status an +explicit, queryable field instead. Every existing zero-project list becomes public, +except a per-algorithm category-map list (name starting with "Taxa returned by"), +which stays hidden as it is today — those lists are an internal bookkeeping detail, +not something meant for every project to browse or attach. + +``projects`` becomes optional (``blank=True``) since a public list no longer needs +a project association to exist. +""" + +from django.db import migrations, models +from django.db.models import Count + + +def backfill_is_public(apps, schema_editor): + TaxaList = apps.get_model("main", "TaxaList") + zero_project_lists = TaxaList.objects.annotate(project_count=Count("projects")).filter(project_count=0) + zero_project_lists.exclude(name__startswith="Taxa returned by").update(is_public=True) + + +def reverse_noop(apps, schema_editor): + # Not reversible: we don't track which rows were public before this migration. + pass + + +class Migration(migrations.Migration): + dependencies = [ + ("main", "0095_grant_sync_deployment_to_mldatamanager"), + ] + + operations = [ + migrations.AlterModelOptions( + name="taxalist", + options={ + "ordering": ["-created_at"], + "permissions": [("manage_public_taxalist", "Can create, edit and delete public taxa lists")], + "verbose_name_plural": "Taxa Lists", + }, + ), + migrations.AddField( + model_name="taxalist", + name="is_public", + field=models.BooleanField( + default=False, + help_text="Public lists are available to every project, not just the ones in 'projects'.", + ), + ), + migrations.AlterField( + model_name="taxalist", + name="projects", + field=models.ManyToManyField(blank=True, related_name="taxa_lists", to="main.project"), + ), + migrations.RunPython(backfill_is_public, reverse_noop), + ] diff --git a/ami/main/models.py b/ami/main/models.py index 9943c111a..f3112ef44 100644 --- a/ami/main/models.py +++ b/ami/main/models.py @@ -4729,16 +4729,19 @@ def save(self, update_calculated_fields=True, *args, **kwargs): class TaxaListQuerySet(BaseQuerySet): def get_or_create_for_project( - self, name: str, project: "Project | None" = None, **defaults + self, name: str, project: "Project | None" = None, is_public: bool = False, **defaults ) -> tuple["TaxaList", bool]: """ Get or create a TaxaList with uniqueness scoped to project. - - If project is None: looks for/creates a global list (no project associations) - - If project is provided: looks for/creates a list associated with that project + - project is given: looks for/creates a list associated with that project (``is_public`` is ignored) + - project is None, is_public=True: looks for/creates the public list with this name + - project is None, is_public=False: looks for/creates a hidden list with no project :param name: Name of the taxa list. - :param project: Project to scope the list to, or None for a global list. + :param project: Project to scope the list to, or None for a list with no project. + :param is_public: When ``project`` is None, whether the list is the public list of this + name or a hidden one (e.g. a per-algorithm category-map list). Ignored otherwise. :param defaults: Extra field values applied only when creating a new list (ignored on the get path, matching Django's ``get_or_create`` semantics). @@ -4749,18 +4752,23 @@ def get_or_create_for_project( Returns: Tuple of (TaxaList, created: bool) """ - if project is None: - # Global list: find list with this name that has no project associations - qs = self.filter(name=name).annotate(project_count=models.Count("projects")).filter(project_count=0) - else: + if project is not None: # Project-specific: find list with this name in this project qs = self.filter(name=name, projects=project) + else: + # No project: find a list with this name and no project associations, + # matching the requested public/hidden state. + qs = ( + self.filter(name=name, is_public=is_public) + .annotate(project_count=models.Count("projects")) + .filter(project_count=0) + ) try: return qs.get(), False except self.model.DoesNotExist: with transaction.atomic(): - taxa_list = self.create(name=name, **defaults) + taxa_list = self.create(name=name, is_public=is_public if project is None else False, **defaults) if project: taxa_list.projects.add(project) return taxa_list, True @@ -4783,13 +4791,18 @@ class TaxaList(BaseModel): description = models.TextField(blank=True) taxa = models.ManyToManyField(Taxon, related_name="lists") - projects = models.ManyToManyField("Project", related_name="taxa_lists") + projects = models.ManyToManyField("Project", related_name="taxa_lists", blank=True) + is_public = models.BooleanField( + default=False, + help_text="Public lists are available to every project, not just the ones in 'projects'.", + ) objects: TaxaListManager = TaxaListManager() class Meta: ordering = ["-created_at"] verbose_name_plural = "Taxa Lists" + permissions = [("manage_public_taxalist", "Can create, edit and delete public taxa lists")] @final From ca0416198b556fc5d68aec649f6ccd387c6d803f Mon Sep 17 00:00:00 2001 From: Michael Bunsen Date: Thu, 17 Sep 2026 14:01:57 -0700 Subject: [PATCH 02/15] feat(taxa): gate public taxa list writes behind a platform permission MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Being a member of a project is no longer enough to edit or delete a taxa list, or change its taxa, once that list is public — only a user with the manage_public_taxalist permission (or a superuser) can. Project-scoped lists keep working exactly as before for their own members. IsProjectMemberOrPublicListManagerOrReadOnly relaxes has_permission() for the manage_public_taxalist bypass on update/destroy only (create still needs real project membership, since there's no object yet to check), then has_object_permission() re-verifies once the target list's is_public flag is known. This closes a gap the relaxed has_permission() would otherwise open: without the object-level re-check, a manage_public_taxalist holder who isn't a project member could edit an unrelated project's own list. The nested add/remove-taxon route has no automatic object-permission check (it never calls get_object()), so it runs the same check_taxalist_write_ permission() by hand after resolving the target list. TaxaListViewSet.get_queryset() takes an include_public param (default true, SingleParamSerializer-validated so a bad value is a 400) and now includes public lists via for_project(). The serializer exposes is_public read-only, and add_m2m_object_permissions() reports update/delete for a public list based on the platform permission instead of project membership. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01TngW6AshkUEhNp9D6z3U9Z --- ami/base/permissions.py | 82 ++++++++++++++++++++++++++++++++++++- ami/main/admin.py | 4 +- ami/main/api/serializers.py | 2 + ami/main/api/views.py | 54 ++++++++++++++++++++---- 4 files changed, 131 insertions(+), 11 deletions(-) diff --git a/ami/base/permissions.py b/ami/base/permissions.py index 247d5e5d4..0b0222b5f 100644 --- a/ami/base/permissions.py +++ b/ami/base/permissions.py @@ -77,6 +77,27 @@ def add_collection_level_permissions(user: User | None, response_data: dict, mod return response_data +def user_can_manage_public_taxalist(user: AbstractBaseUser | AnonymousUser) -> bool: + """A superuser, or a user holding the platform-wide manage_public_taxalist permission.""" + if not user or not user.is_authenticated: + return False + return bool(user.is_superuser or user.has_perm("main.manage_public_taxalist")) # type: ignore[union-attr] + + +def check_taxalist_write_permission(user, taxa_list, project) -> bool: + """ + True if `user` may update/delete `taxa_list` or change its taxa. + + A public list requires the platform-wide manage_public_taxalist permission + (superusers always pass); a project-scoped list requires membership of `project`. + """ + if user.is_superuser: + return True + if getattr(taxa_list, "is_public", False): + return user.has_perm("main.manage_public_taxalist") + return bool(project and project.members.filter(pk=user.pk).exists()) + + def add_m2m_object_permissions(user, instance, project, response_data: dict) -> dict: """ Add object-level permissions for models with an M2M relationship to Project. @@ -87,7 +108,9 @@ def add_m2m_object_permissions(user, instance, project, response_data: dict) -> against a specific project from the request context instead. Validates that the instance actually belongs to the given project before - granting any permissions (prevents cross-project permission leaks). + granting any permissions (prevents cross-project permission leaks). A public + instance is the one exception: its update/delete permissions come from the + platform-wide manage_public_taxalist permission, not project membership. This is a temporary approach for the M2M permission gap described in #1120. Once that issue is resolved, this should be replaced by a generic permission @@ -96,6 +119,12 @@ class (Pattern B: Bare M2M) that handles TaxaList, Taxon, ProcessingService, """ perms = set(response_data.get("user_permissions", [])) + if getattr(instance, "is_public", False): + if user_can_manage_public_taxalist(user): + perms.update(["update", "delete"]) + response_data["user_permissions"] = list(perms) + return response_data + if not project or not instance.projects.filter(pk=project.pk).exists(): response_data["user_permissions"] = list(perms) return response_data @@ -144,6 +173,57 @@ def has_permission(self, request, view): return project.members.filter(pk=request.user.pk).exists() +class IsProjectMemberOrPublicListManager(permissions.BasePermission): + """ + Like IsProjectMemberOrReadOnly, but a user holding the platform-wide + manage_public_taxalist permission also passes, regardless of project + membership — for actions that can only ever land on a public list (adding + or removing a taxon from an existing TaxaList via the nested route). + """ + + def has_permission(self, request, view): + if request.method in permissions.SAFE_METHODS: + return True + + if not request.user or not request.user.is_authenticated: + return False + + if request.user.is_superuser or user_can_manage_public_taxalist(request.user): # type: ignore[union-attr] + return True + + get_active_project = getattr(view, "get_active_project", None) + if not get_active_project: + return False + + project = get_active_project() + if not project: + return False + + return project.members.filter(pk=request.user.pk).exists() + + +class IsProjectMemberOrPublicListManagerOrReadOnly(IsProjectMemberOrPublicListManager): + """ + For TaxaListViewSet: creating a brand-new list is always project-scoped and + needs real project membership (there's no object yet to tell whether it will + be public). Update/delete defer to the object-level check below, which grants + the manage_public_taxalist bypass only once the target list's is_public flag + is known — closing the gap where a project member could edit a public list + just by supplying their own project_id. + """ + + def has_permission(self, request, view): + if getattr(view, "action", None) == "create": + return IsProjectMemberOrReadOnly.has_permission(self, request, view) + return super().has_permission(request, view) + + def has_object_permission(self, request, view, obj): + if request.method in permissions.SAFE_METHODS: + return True + project = view.get_active_project() if hasattr(view, "get_active_project") else None + return check_taxalist_write_permission(request.user, obj, project) + + class ObjectPermission(permissions.BasePermission): """ Generic permission class that delegates to the model's `check_permission(user, action)` method. diff --git a/ami/main/admin.py b/ami/main/admin.py index 55c84701f..88bf8169c 100644 --- a/ami/main/admin.py +++ b/ami/main/admin.py @@ -736,7 +736,7 @@ def parent_names(self, obj) -> str: class TaxaListAdmin(admin.ModelAdmin[TaxaList]): """Admin panel example for ``TaxaList`` model.""" - list_display = ("name", "taxa_count", "created_at", "updated_at") + list_display = ("name", "is_public", "taxa_count", "created_at", "updated_at") def taxa_count(self, obj) -> int: return obj.taxa.count() @@ -746,7 +746,7 @@ def taxa_count(self, obj) -> int: "projects", ) - list_filter = ("projects",) + list_filter = ("is_public", "projects") @admin.register(Device) diff --git a/ami/main/api/serializers.py b/ami/main/api/serializers.py index 952f936f0..c18cb395a 100644 --- a/ami/main/api/serializers.py +++ b/ami/main/api/serializers.py @@ -708,6 +708,7 @@ class TaxaListSerializer(DefaultSerializer): taxa = serializers.SerializerMethodField() taxa_count = serializers.SerializerMethodField() projects = serializers.SerializerMethodField() + is_public = serializers.BooleanField(read_only=True) class Meta: model = TaxaList @@ -718,6 +719,7 @@ class Meta: "taxa", "taxa_count", "projects", + "is_public", "created_at", "updated_at", ] diff --git a/ami/main/api/views.py b/ami/main/api/views.py index 5fe6cf863..8adc72507 100644 --- a/ami/main/api/views.py +++ b/ami/main/api/views.py @@ -29,7 +29,13 @@ from ami.base.metadata import ResponseSchemaMetadata from ami.base.models import BaseQuerySet from ami.base.pagination import LimitOffsetPaginationWithPermissions -from ami.base.permissions import IsActiveStaffOrReadOnly, IsProjectMemberOrReadOnly, ObjectPermission +from ami.base.permissions import ( + IsActiveStaffOrReadOnly, + IsProjectMemberOrPublicListManager, + IsProjectMemberOrPublicListManagerOrReadOnly, + ObjectPermission, + check_taxalist_write_permission, +) from ami.base.serializers import FilterParamsSerializer, SingleParamSerializer from ami.base.views import ProjectMixin from ami.main.api.schemas import limit_doc_param, project_id_doc_param @@ -2209,7 +2215,7 @@ class TaxaListViewSet(DefaultViewSet, ProjectMixin): "created_at", "updated_at", ] - permission_classes = [IsProjectMemberOrReadOnly] + permission_classes = [IsProjectMemberOrPublicListManagerOrReadOnly] require_project = True def get_queryset(self): @@ -2217,9 +2223,29 @@ def get_queryset(self): # Annotate with taxa count for better performance qs = qs.annotate(annotated_taxa_count=models.Count("taxa")) project = self.get_active_project() - if project: - return qs.filter(projects=project) - return qs + if not project: + return qs + include_public = SingleParamSerializer[bool].clean( + param_name="include_public", + field=serializers.BooleanField(required=False, default=True), + data=self.request.query_params, + ) + return qs.for_project(project, include_public=include_public) + + @extend_schema( + parameters=[ + project_id_doc_param, + OpenApiParameter( + name="include_public", + description="Include taxa lists that are public (available to every project), " + "not just the ones belonging to this project. Defaults to true.", + required=False, + type=OpenApiTypes.BOOL, + ), + ] + ) + def list(self, request, *args, **kwargs): + return super().list(request, *args, **kwargs) def perform_create(self, serializer): """ @@ -2244,18 +2270,28 @@ class TaxaListTaxonViewSet(viewsets.GenericViewSet, ProjectMixin): """ serializer_class = TaxaListTaxonSerializer - permission_classes = [IsProjectMemberOrReadOnly] + permission_classes = [IsProjectMemberOrPublicListManager] require_project = True def get_taxa_list(self): - """Get the parent taxa list from URL parameters, scoped to the active project.""" + """Get the parent taxa list, scoped to the active project or public.""" taxa_list_id = self.kwargs.get("taxalist_pk") project = self.get_active_project() try: - return TaxaList.objects.get(pk=taxa_list_id, projects=project) + return TaxaList.objects.for_project(project, include_public=True).get(pk=taxa_list_id) except TaxaList.DoesNotExist: raise api_exceptions.NotFound("Taxa list not found.") from None + def check_write_permission(self, taxa_list): + """ + Re-check against the actual list: IsProjectMemberOrPublicListManager only + gates coarsely at has_permission() time, before the target list (and its + is_public flag) is known. + """ + project = self.get_active_project() + if not check_taxalist_write_permission(self.request.user, taxa_list, project): + raise api_exceptions.PermissionDenied("You do not have permission to modify this taxa list.") + def get_queryset(self): """Return taxa in the specified taxa list.""" taxa_list = self.get_taxa_list() @@ -2264,6 +2300,7 @@ def get_queryset(self): def create(self, request, taxalist_pk=None): """Add a taxon to the taxa list.""" taxa_list = self.get_taxa_list() + self.check_write_permission(taxa_list) # Validate input input_serializer = TaxaListTaxonInputSerializer(data=request.data) @@ -2292,6 +2329,7 @@ def delete_by_taxon(self, request, taxalist_pk=None, taxon_id=None): DELETE /taxa/lists/{taxa_list_id}/taxa/{taxon_id}/ """ taxa_list = self.get_taxa_list() + self.check_write_permission(taxa_list) # Check if taxon exists in list if not taxa_list.taxa.filter(pk=taxon_id).exists(): From 42fda58bf83144bda0cc9ad71aa9319cb244ef51 Mon Sep 17 00:00:00 2001 From: Michael Bunsen Date: Thu, 17 Sep 2026 14:02:14 -0700 Subject: [PATCH 03/15] refactor(taxa): make imported/updated taxa lists public, not just hidden import_taxa and update_taxa create their taxa list with project=None, which used to mean an ad hoc hidden list with no project. Now that public is an explicit flag, pass is_public=True so those lists are actually available to every project, matching what "global list" meant in comments up to now. Also rewords the per-algorithm category-map list's comment in pipeline.py away from "global", since that word now means something more specific. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01TngW6AshkUEhNp9D6z3U9Z --- ami/main/management/commands/import_taxa.py | 5 +++-- ami/main/management/commands/update_taxa.py | 7 +++++-- ami/ml/models/pipeline.py | 2 +- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/ami/main/management/commands/import_taxa.py b/ami/main/management/commands/import_taxa.py index a1f9cf49b..31f393677 100644 --- a/ami/main/management/commands/import_taxa.py +++ b/ami/main/management/commands/import_taxa.py @@ -236,8 +236,9 @@ def handle(self, *args, **options): else: list_name = pathlib.Path(fname).stem - # Uses get_or_create_for_project with project=None to create a global list - taxalist, created = TaxaList.objects.get_or_create_for_project(name=list_name, project=None) + # Uses get_or_create_for_project with project=None to create a public list + # available to every project. + taxalist, created = TaxaList.objects.get_or_create_for_project(name=list_name, project=None, is_public=True) if created: self.stdout.write(self.style.SUCCESS('Successfully created taxa list "%s"' % taxalist)) diff --git a/ami/main/management/commands/update_taxa.py b/ami/main/management/commands/update_taxa.py index 92779fcbc..6564f21e0 100644 --- a/ami/main/management/commands/update_taxa.py +++ b/ami/main/management/commands/update_taxa.py @@ -127,11 +127,14 @@ def handle(self, *args, **options): incoming_taxa = read_csv(fname) # Get or create taxa list if specified - # Uses get_or_create_for_project with project=None to create a global list + # Uses get_or_create_for_project with project=None to create a public list + # available to every project. taxalist = None if options["list"]: list_name = options["list"] - taxalist, created = TaxaList.objects.get_or_create_for_project(name=list_name, project=None) + taxalist, created = TaxaList.objects.get_or_create_for_project( + name=list_name, project=None, is_public=True + ) if created: self.stdout.write(self.style.SUCCESS(f"Created new taxa list '{list_name}'")) else: diff --git a/ami/ml/models/pipeline.py b/ami/ml/models/pipeline.py index 8ebaf6d6d..ddc43ef2e 100644 --- a/ami/ml/models/pipeline.py +++ b/ami/ml/models/pipeline.py @@ -723,7 +723,7 @@ def get_or_create_taxon_for_classification( """ taxa_list, created = TaxaList.objects.get_or_create_for_project( name=f"Taxa returned by {algorithm.name}", - project=None, # Algorithm taxa lists are global + project=None, # Algorithm taxa lists have no project and stay hidden, not public ) if created: logger.info(f"Created new taxa list {taxa_list}") From c5d05eccd6f631ffd787ebd51326574bdff0b9c2 Mon Sep 17 00:00:00 2001 From: Michael Bunsen Date: Thu, 17 Sep 2026 14:02:23 -0700 Subject: [PATCH 04/15] =?UTF-8?q?test(taxa):=20cover=20public=20taxa=20lis?= =?UTF-8?q?ts=20=E2=80=94=20permissions,=20visibility,=20and=20query=20cos?= =?UTF-8?q?t?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the permission matrix for public vs. project-scoped TaxaLists (member, non-member, anonymous, superuser, and a plain manage_public_taxalist holder) across update, delete, and add/remove taxon; the include_public query param and its 400 on an invalid value; draft-project visibility for a non-public list; a multi-row assertNumQueries check on the list endpoint; direct unit tests for BaseQuerySet.for_project(); and the 0096 migration's backfill rule. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01TngW6AshkUEhNp9D6z3U9Z --- ami/main/tests.py | 373 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 372 insertions(+), 1 deletion(-) diff --git a/ami/main/tests.py b/ami/main/tests.py index 3c9a1be17..2c887056e 100644 --- a/ami/main/tests.py +++ b/ami/main/tests.py @@ -6,7 +6,7 @@ from unittest import mock from django.conf import settings -from django.contrib.auth.models import AnonymousUser +from django.contrib.auth.models import AnonymousUser, Permission from django.core.files.uploadedfile import SimpleUploadedFile from django.db import IntegrityError, connection, models from django.test import TestCase, override_settings @@ -5416,6 +5416,377 @@ def test_defaults_applied_on_create_only(self): self.assertEqual(taxa_list_again.pk, taxa_list.pk) self.assertEqual(taxa_list_again.description, "Initial description") + def test_creates_new_public_list(self): + taxa_list, created = TaxaList.objects.get_or_create_for_project( + name="Public Moths", project=None, is_public=True + ) + self.assertTrue(created) + self.assertTrue(taxa_list.is_public) + self.assertEqual(taxa_list.projects.count(), 0) + + def test_retrieves_existing_public_list(self): + existing = TaxaList.objects.create(name="Public Moths", is_public=True) + taxa_list, created = TaxaList.objects.get_or_create_for_project( + name="Public Moths", project=None, is_public=True + ) + self.assertFalse(created) + self.assertEqual(taxa_list.pk, existing.pk) + + def test_public_and_non_public_no_project_lists_with_same_name_do_not_collide(self): + """A public list and a hidden (non-public, no-project) list can share a name.""" + public_list, public_created = TaxaList.objects.get_or_create_for_project( + name="Moths No Project", project=None, is_public=True + ) + hidden_list, hidden_created = TaxaList.objects.get_or_create_for_project( + name="Moths No Project", project=None, is_public=False + ) + self.assertTrue(public_created) + self.assertTrue(hidden_created) + self.assertNotEqual(public_list.pk, hidden_list.pk) + self.assertTrue(public_list.is_public) + self.assertFalse(hidden_list.is_public) + + +class TaxaListForProjectQuerySetTestCase(TestCase): + """Direct unit tests for BaseQuerySet.for_project(), independent of the API layer.""" + + def setUp(self): + self.project_a = Project.objects.create(name="FP Project A") + self.project_b = Project.objects.create(name="FP Project B") + self.scoped = TaxaList.objects.create(name="FP Scoped") + self.scoped.projects.add(self.project_a) + self.public = TaxaList.objects.create(name="FP Public", is_public=True) + self.hidden = TaxaList.objects.create(name="FP Hidden") # is_public=False, no projects + + def test_returns_project_scoped_and_public_by_default(self): + ids = set(TaxaList.objects.for_project(self.project_a).values_list("pk", flat=True)) + self.assertEqual(ids, {self.scoped.pk, self.public.pk}) + + def test_excludes_public_when_include_public_false(self): + ids = set(TaxaList.objects.for_project(self.project_a, include_public=False).values_list("pk", flat=True)) + self.assertEqual(ids, {self.scoped.pk}) + + def test_excludes_lists_scoped_to_other_projects(self): + ids = set(TaxaList.objects.for_project(self.project_b).values_list("pk", flat=True)) + self.assertNotIn(self.scoped.pk, ids) + + def test_public_list_linked_to_project_is_not_duplicated(self): + self.public.projects.add(self.project_a) + rows = list(TaxaList.objects.for_project(self.project_a).filter(pk=self.public.pk)) + self.assertEqual(len(rows), 1) + + def test_raises_on_model_without_m2m_projects_field(self): + with self.assertRaises(TypeError): + list(Event.objects.for_project(self.project_a)) + + +class TaxaListIsPublicBackfillTestCase(TestCase): + """Unit test for the 0096 migration's is_public backfill rule.""" + + def _backfill(self): + from importlib import import_module + + from django.apps import apps as real_apps + + mod = import_module("ami.main.migrations.0096_taxalist_is_public") + mod.backfill_is_public(real_apps, None) + + def test_zero_project_list_becomes_public(self): + taxa_list = TaxaList.objects.create(name="No Project List") + self._backfill() + taxa_list.refresh_from_db() + self.assertTrue(taxa_list.is_public) + + def test_project_scoped_list_stays_non_public(self): + project = Project.objects.create(name="Backfill Project") + taxa_list = TaxaList.objects.create(name="Scoped List") + taxa_list.projects.add(project) + self._backfill() + taxa_list.refresh_from_db() + self.assertFalse(taxa_list.is_public) + + def test_algorithm_category_map_list_stays_non_public(self): + taxa_list = TaxaList.objects.create(name="Taxa returned by Some Algorithm") + self._backfill() + taxa_list.refresh_from_db() + self.assertFalse(taxa_list.is_public) + + +class TaxaListPublicPermissionsTestCase(TestCase): + """Permission matrix for public vs. project-scoped TaxaLists. + + The key regression this protects against: a project member passing their own + project_id must not be able to modify, delete, or change the taxa of a public + list. Only the manage_public_taxalist platform permission (or a superuser) can. + A project-scoped list keeps its existing member-can-write behavior. + """ + + def setUp(self): + self.owner = User.objects.create_user(email="owner-pub@example.com", password="testpass") + self.member = User.objects.create_user(email="member-pub@example.com", password="testpass") + self.non_member = User.objects.create_user(email="nonmember-pub@example.com", password="testpass") + self.superuser = User.objects.create_superuser(email="super-pub@example.com", password="testpass") + self.public_manager = User.objects.create_user(email="manager-pub@example.com", password="testpass") + perm = Permission.objects.get(codename="manage_public_taxalist", content_type__app_label="main") + self.public_manager.user_permissions.add(perm) + + self.project = Project.objects.create(name="Test Project", owner=self.owner) + self.project.members.add(self.member) + + self.public_list = TaxaList.objects.create(name="Public List", is_public=True) + self.scoped_list = TaxaList.objects.create(name="Scoped List") + self.scoped_list.projects.add(self.project) + + self.taxon = Taxon.objects.create(name="Test Taxon", rank="SPECIES") + + self.client = APIClient() + + def _detail_url(self, taxa_list): + return f"/api/v2/taxa/lists/{taxa_list.pk}/?project_id={self.project.pk}" + + def _taxa_url(self, taxa_list): + return f"/api/v2/taxa/lists/{taxa_list.pk}/taxa/?project_id={self.project.pk}" + + def _taxon_detail_url(self, taxa_list, taxon): + return f"/api/v2/taxa/lists/{taxa_list.pk}/taxa/{taxon.pk}/?project_id={self.project.pk}" + + # -- Update -- + + def test_member_cannot_update_public_list(self): + self.client.force_authenticate(self.member) + response = self.client.patch(self._detail_url(self.public_list), {"name": "Hacked"}) + self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) + + def test_non_member_cannot_update_public_list(self): + self.client.force_authenticate(self.non_member) + response = self.client.patch(self._detail_url(self.public_list), {"name": "Hacked"}) + self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) + + def test_anonymous_cannot_update_public_list(self): + response = self.client.patch(self._detail_url(self.public_list), {"name": "Hacked"}) + self.assertIn(response.status_code, [status.HTTP_401_UNAUTHORIZED, status.HTTP_403_FORBIDDEN]) + + def test_public_list_manager_can_update_public_list(self): + self.client.force_authenticate(self.public_manager) + response = self.client.patch(self._detail_url(self.public_list), {"name": "Renamed"}) + self.assertEqual(response.status_code, status.HTTP_200_OK) + + def test_superuser_can_update_public_list(self): + self.client.force_authenticate(self.superuser) + response = self.client.patch(self._detail_url(self.public_list), {"name": "Renamed by super"}) + self.assertEqual(response.status_code, status.HTTP_200_OK) + + def test_member_can_still_update_scoped_list(self): + self.client.force_authenticate(self.member) + response = self.client.patch(self._detail_url(self.scoped_list), {"name": "Renamed"}) + self.assertEqual(response.status_code, status.HTTP_200_OK) + + def test_public_list_manager_without_membership_cannot_update_scoped_list(self): + """Holding manage_public_taxalist does not grant control over project-scoped lists.""" + self.client.force_authenticate(self.public_manager) + response = self.client.patch(self._detail_url(self.scoped_list), {"name": "Hacked"}) + self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) + + # -- Delete -- + + def test_member_cannot_delete_public_list(self): + self.client.force_authenticate(self.member) + response = self.client.delete(self._detail_url(self.public_list)) + self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) + + def test_public_list_manager_can_delete_public_list(self): + self.client.force_authenticate(self.public_manager) + response = self.client.delete(self._detail_url(self.public_list)) + self.assertEqual(response.status_code, status.HTTP_204_NO_CONTENT) + + def test_member_can_still_delete_scoped_list(self): + self.client.force_authenticate(self.member) + response = self.client.delete(self._detail_url(self.scoped_list)) + self.assertEqual(response.status_code, status.HTTP_204_NO_CONTENT) + + # -- Retrieve / list visibility -- + + def test_anonymous_can_retrieve_public_list(self): + response = self.client.get(self._detail_url(self.public_list)) + self.assertEqual(response.status_code, status.HTTP_200_OK) + + def test_non_member_can_see_public_list(self): + """A public list is visible even to a user with no connection to the active project. + + The project here is non-draft, so its own scoped_list is also visible to + anyone by the platform's general draft-visibility rule — that is separate + from is_public and is covered by TaxaListDraftProjectVisibilityTestCase. + """ + self.client.force_authenticate(self.non_member) + response = self.client.get(f"/api/v2/taxa/lists/?project_id={self.project.pk}") + self.assertEqual(response.status_code, status.HTTP_200_OK) + ids = {row["id"] for row in response.json()["results"]} + self.assertIn(self.public_list.pk, ids) + + def test_is_public_field_is_read_only_and_reported(self): + self.client.force_authenticate(self.member) + response = self.client.get(self._detail_url(self.public_list)) + self.assertTrue(response.json()["is_public"]) + + response = self.client.patch(self._detail_url(self.scoped_list), {"is_public": True}) + self.scoped_list.refresh_from_db() + self.assertFalse(self.scoped_list.is_public) + + def test_user_permissions_include_update_delete_for_public_manager_only(self): + self.client.force_authenticate(self.member) + response = self.client.get(self._detail_url(self.public_list)) + self.assertNotIn("update", response.json()["user_permissions"]) + + self.client.force_authenticate(self.public_manager) + response = self.client.get(self._detail_url(self.public_list)) + perms = response.json()["user_permissions"] + self.assertIn("update", perms) + self.assertIn("delete", perms) + + # -- Add / remove taxon (nested route) -- + + def test_member_cannot_add_taxon_to_public_list(self): + self.client.force_authenticate(self.member) + response = self.client.post(self._taxa_url(self.public_list), {"taxon_id": self.taxon.pk}) + self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) + self.assertFalse(self.public_list.taxa.filter(pk=self.taxon.pk).exists()) + + def test_public_list_manager_can_add_taxon_to_public_list(self): + self.client.force_authenticate(self.public_manager) + response = self.client.post(self._taxa_url(self.public_list), {"taxon_id": self.taxon.pk}) + self.assertEqual(response.status_code, status.HTTP_201_CREATED) + self.assertTrue(self.public_list.taxa.filter(pk=self.taxon.pk).exists()) + + def test_member_cannot_remove_taxon_from_public_list(self): + self.public_list.taxa.add(self.taxon) + self.client.force_authenticate(self.member) + response = self.client.delete(self._taxon_detail_url(self.public_list, self.taxon)) + self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) + self.assertTrue(self.public_list.taxa.filter(pk=self.taxon.pk).exists()) + + def test_public_list_manager_can_remove_taxon_from_public_list(self): + self.public_list.taxa.add(self.taxon) + self.client.force_authenticate(self.public_manager) + response = self.client.delete(self._taxon_detail_url(self.public_list, self.taxon)) + self.assertEqual(response.status_code, status.HTTP_204_NO_CONTENT) + self.assertFalse(self.public_list.taxa.filter(pk=self.taxon.pk).exists()) + + def test_member_can_still_add_taxon_to_scoped_list(self): + self.client.force_authenticate(self.member) + response = self.client.post(self._taxa_url(self.scoped_list), {"taxon_id": self.taxon.pk}) + self.assertEqual(response.status_code, status.HTTP_201_CREATED) + + def test_public_list_manager_without_membership_cannot_add_taxon_to_scoped_list(self): + self.client.force_authenticate(self.public_manager) + response = self.client.post(self._taxa_url(self.scoped_list), {"taxon_id": self.taxon.pk}) + self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) + + +class TaxaListIncludePublicParamTestCase(TestCase): + """?include_public toggles whether public lists appear alongside a project's own lists.""" + + def setUp(self): + self.user = User.objects.create_user(email="scope-user@example.com", password="testpass") + self.project = Project.objects.create(name="Scope Project", owner=self.user) + self.other_project = Project.objects.create(name="Other Scope Project") + + self.scoped_list = TaxaList.objects.create(name="Scoped") + self.scoped_list.projects.add(self.project) + self.public_list = TaxaList.objects.create(name="Public", is_public=True) + # A public list can also be linked to an unrelated project without appearing twice. + self.public_list.projects.add(self.other_project) + + self.hidden_list = TaxaList.objects.create(name="Hidden with no project") + + self.client = APIClient() + self.client.force_authenticate(self.user) + + def _list_ids(self, **params): + params["project_id"] = self.project.pk + query = "&".join(f"{k}={v}" for k, v in params.items()) + response = self.client.get(f"/api/v2/taxa/lists/?{query}") + self.assertEqual(response.status_code, status.HTTP_200_OK) + return response.json()["results"] + + def test_public_lists_included_by_default(self): + ids = {row["id"] for row in self._list_ids()} + self.assertEqual(ids, {self.scoped_list.pk, self.public_list.pk}) + + def test_include_public_false_hides_public_lists(self): + ids = {row["id"] for row in self._list_ids(include_public="false")} + self.assertEqual(ids, {self.scoped_list.pk}) + + def test_include_public_invalid_value_returns_400(self): + response = self.client.get(f"/api/v2/taxa/lists/?project_id={self.project.pk}&include_public=notabool") + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + + def test_public_list_linked_to_multiple_projects_appears_once(self): + """A public list attached to several projects must not be duplicated by the M2M join.""" + self.public_list.projects.add(self.project) + rows = self._list_ids() + matches = [row for row in rows if row["id"] == self.public_list.pk] + self.assertEqual(len(matches), 1) + + def test_hidden_zero_project_list_is_invisible(self): + ids = {row["id"] for row in self._list_ids()} + self.assertNotIn(self.hidden_list.pk, ids) + + +class TaxaListDraftProjectVisibilityTestCase(TestCase): + """A non-public list in a draft project follows the same visibility rule as any + other draft-project object: only members, owners and superusers can see it.""" + + def setUp(self): + self.owner = User.objects.create_user(email="draft-owner@example.com", password="testpass") + self.member = User.objects.create_user(email="draft-member@example.com", password="testpass") + self.outsider = User.objects.create_user(email="draft-outsider@example.com", password="testpass") + self.draft_project = Project.objects.create(name="Draft Project", owner=self.owner, draft=True) + self.draft_project.members.add(self.member) + self.scoped_list = TaxaList.objects.create(name="Draft List") + self.scoped_list.projects.add(self.draft_project) + self.client = APIClient() + + def _list_url(self): + return f"/api/v2/taxa/lists/?project_id={self.draft_project.pk}" + + def test_outsider_cannot_see_list_in_draft_project(self): + self.client.force_authenticate(self.outsider) + response = self.client.get(self._list_url()) + ids = {row["id"] for row in response.json()["results"]} + self.assertNotIn(self.scoped_list.pk, ids) + + def test_member_can_see_list_in_draft_project(self): + self.client.force_authenticate(self.member) + response = self.client.get(self._list_url()) + ids = {row["id"] for row in response.json()["results"]} + self.assertIn(self.scoped_list.pk, ids) + + +@override_settings(CACHALOT_ENABLED=False) +class TaxaListQueryCountTestCase(APITestCase): + """Audit TaxaListViewSet.list for N+1 across a mixed public/scoped, multi-row fixture.""" + + def setUp(self): + self.user = User.objects.create_user(email="qc-user@example.com", password="testpass") + self.project = Project.objects.create(name="QC Project", owner=self.user) + for i in range(3): + scoped = TaxaList.objects.create(name=f"Scoped {i}") + scoped.projects.add(self.project) + for i in range(2): + TaxaList.objects.create(name=f"Public {i}", is_public=True) + self.client = APIClient() + self.client.force_authenticate(self.user) + + def test_list_query_count(self): + from cachalot.api import cachalot_disabled + + url = f"/api/v2/taxa/lists/?project_id={self.project.pk}" + with cachalot_disabled(), CaptureQueriesContext(connection) as ctx: + response = self.client.get(url) + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(len(response.json()["results"]), 5) + self.assertEqual(len(ctx.captured_queries), 34) + class TaxaListDedupeMigrationTestCase(TestCase): """Exercise the 0083_dedupe_taxalist_names data migration logic against seeded duplicates.""" From 17afebf2c501f8fbae3430c97731c1496ac31440 Mon Sep 17 00:00:00 2001 From: Michael Bunsen Date: Thu, 17 Sep 2026 14:35:47 -0700 Subject: [PATCH 05/15] fix(taxa): rewrite the migration docstring, stop leaking a draft list's existence via the nested route MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 0096 migration docstring described "Taxa returned by " lists as a category-map detail — they're not: each one holds only the taxa that algorithm has actually returned as a top prediction so far. Rewrites the docstring to say what is_public means, the backfill rule, and that these lists stay non-public because what a project should see or do with them is still undecided. TaxaListTaxonViewSet.get_taxa_list() resolved the parent list via for_project() alone, which does not apply draft-project visibility. A manage_public_taxalist holder bypasses the project-membership check at has_permission() (the target might turn out to be public), so without this fix they could find out a project-scoped list exists in a draft project they have nothing to do with, just by guessing its id and that project's id. get_taxa_list() now runs visible_for_user() first, so a non-public list in an unrelated draft project resolves to 404 for anyone but its members and superusers. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01TngW6AshkUEhNp9D6z3U9Z --- ami/main/api/views.py | 6 ++- .../migrations/0096_taxalist_is_public.py | 18 +++----- ami/main/tests.py | 44 +++++++++++++++++++ 3 files changed, 56 insertions(+), 12 deletions(-) diff --git a/ami/main/api/views.py b/ami/main/api/views.py index 8adc72507..09f1abb44 100644 --- a/ami/main/api/views.py +++ b/ami/main/api/views.py @@ -2278,7 +2278,11 @@ def get_taxa_list(self): taxa_list_id = self.kwargs.get("taxalist_pk") project = self.get_active_project() try: - return TaxaList.objects.for_project(project, include_public=True).get(pk=taxa_list_id) + return ( + TaxaList.objects.visible_for_user(self.request.user) + .for_project(project, include_public=True) + .get(pk=taxa_list_id) + ) except TaxaList.DoesNotExist: raise api_exceptions.NotFound("Taxa list not found.") from None diff --git a/ami/main/migrations/0096_taxalist_is_public.py b/ami/main/migrations/0096_taxalist_is_public.py index f97b6eaf1..650f40f23 100644 --- a/ami/main/migrations/0096_taxalist_is_public.py +++ b/ami/main/migrations/0096_taxalist_is_public.py @@ -1,15 +1,11 @@ """ -Add ``TaxaList.is_public`` and backfill it for existing rows. - -A TaxaList with no project was previously treated as an ad hoc "global" list by -convention (see ``get_or_create_for_project``); this migration makes that status an -explicit, queryable field instead. Every existing zero-project list becomes public, -except a per-algorithm category-map list (name starting with "Taxa returned by"), -which stays hidden as it is today — those lists are an internal bookkeeping detail, -not something meant for every project to browse or attach. - -``projects`` becomes optional (``blank=True``) since a public list no longer needs -a project association to exist. +``TaxaList.is_public`` marks a list as available to every project, not just the +ones in its ``projects`` M2M. This backfills it: every existing list with no +project becomes public, except a "Taxa returned by " list — the +running set of taxa that algorithm has returned as a top prediction — which +stays non-public, since what a project should see or do with that list is +still undecided. ``projects`` becomes ``blank=True`` since a public list no +longer needs one. """ from django.db import migrations, models diff --git a/ami/main/tests.py b/ami/main/tests.py index 2c887056e..b70077c2b 100644 --- a/ami/main/tests.py +++ b/ami/main/tests.py @@ -5762,6 +5762,50 @@ def test_member_can_see_list_in_draft_project(self): self.assertIn(self.scoped_list.pk, ids) +class TaxaListTaxonDraftProjectVisibilityTestCase(TestCase): + """ + A manage_public_taxalist holder bypasses the project-membership check at + has_permission() (the target might turn out to be public), but a + project-scoped list in an unrelated draft project is neither public nor + theirs — get_taxa_list() must still hide it via visible_for_user() rather + than leaking that it exists. A genuine project member can still read/write. + + A plain non-member (no platform permission) never reaches this check at + all: IsProjectMemberOrPublicListManager.has_permission() already denies + them with 403 for not being a member of the active project, regardless of + draft status — that's a separate, pre-existing gate, not this fix. + """ + + def setUp(self): + self.owner = User.objects.create_user(email="taxon-draft-owner@example.com", password="testpass") + self.member = User.objects.create_user(email="taxon-draft-member@example.com", password="testpass") + self.public_list_manager = User.objects.create_user( + email="taxon-draft-manager@example.com", password="testpass" + ) + perm = Permission.objects.get(codename="manage_public_taxalist", content_type__app_label="main") + self.public_list_manager.user_permissions.add(perm) + self.draft_project = Project.objects.create(name="Taxon Draft Project", owner=self.owner, draft=True) + self.draft_project.members.add(self.member) + self.scoped_list = TaxaList.objects.create(name="Taxon Draft List") + self.scoped_list.projects.add(self.draft_project) + self.taxon = Taxon.objects.create(name="Taxon Draft Species", rank="SPECIES") + self.client = APIClient() + + def _taxa_url(self): + return f"/api/v2/taxa/lists/{self.scoped_list.pk}/taxa/?project_id={self.draft_project.pk}" + + def test_public_list_manager_cannot_see_scoped_list_in_unrelated_draft_project(self): + self.client.force_authenticate(self.public_list_manager) + response = self.client.post(self._taxa_url(), {"taxon_id": self.taxon.pk}) + self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) + self.assertFalse(self.scoped_list.taxa.filter(pk=self.taxon.pk).exists()) + + def test_member_can_add_taxon_to_list_in_draft_project(self): + self.client.force_authenticate(self.member) + response = self.client.post(self._taxa_url(), {"taxon_id": self.taxon.pk}) + self.assertEqual(response.status_code, status.HTTP_201_CREATED) + + @override_settings(CACHALOT_ENABLED=False) class TaxaListQueryCountTestCase(APITestCase): """Audit TaxaListViewSet.list for N+1 across a mixed public/scoped, multi-row fixture.""" From 3611963524143124f9b79aee039bd6a4496696ad Mon Sep 17 00:00:00 2001 From: Michael Bunsen Date: Thu, 17 Sep 2026 14:36:03 -0700 Subject: [PATCH 06/15] refactor(base): generalize the public-row write-permission helpers beyond TaxaList user_can_manage_public_taxalist() only ever checked one hardcoded permission string. Replaced it with user_can_manage_public(user, model_or_instance), which derives .manage_public_ from the model's own metadata, so any model with an is_public flag can reuse it instead of every model growing its own copy. add_m2m_object_permissions() and IsProjectMemberOrPublicListManager now call the generic helper. check_taxalist_write_permission() stays TaxaList-specific rather than folding into a shared write-check: its non-public fallback is project membership, while ProcessingService's is active-staff status, so the two bodies would diverge immediately. Added the ProcessingService equivalent, check_processingservice_write_permission(), plus IsActiveStaffOrPublicManager and IsActiveStaffOrPublicManagerOrReadOnly, mirroring the TaxaList permission classes' shape (safe methods open; create always needs the base gate since there's no object yet; update/delete/register_pipelines defer to the object-level check once is_public is known) with staff status as the base gate in place of project membership. Also fixed a docstring on IsProjectMemberOrPublicListManager that overstated its own scope: the nested add/remove-taxon route it guards serves ordinary project-scoped lists too, not only public ones. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01TngW6AshkUEhNp9D6z3U9Z --- ami/base/permissions.py | 111 +++++++++++++++++++++++++++++++--------- 1 file changed, 87 insertions(+), 24 deletions(-) diff --git a/ami/base/permissions.py b/ami/base/permissions.py index 0b0222b5f..02c54d89d 100644 --- a/ami/base/permissions.py +++ b/ami/base/permissions.py @@ -77,25 +77,44 @@ def add_collection_level_permissions(user: User | None, response_data: dict, mod return response_data -def user_can_manage_public_taxalist(user: AbstractBaseUser | AnonymousUser) -> bool: - """A superuser, or a user holding the platform-wide manage_public_taxalist permission.""" +def user_can_manage_public(user: AbstractBaseUser | AnonymousUser, model_or_instance) -> bool: + """ + A superuser, or a user holding .manage_public_ for + the given model (or an instance of it) — the platform permission gating + write access to a public row, in place of project membership or staff status. + """ if not user or not user.is_authenticated: return False - return bool(user.is_superuser or user.has_perm("main.manage_public_taxalist")) # type: ignore[union-attr] + if user.is_superuser: # type: ignore[union-attr] + return True + meta = model_or_instance._meta + return user.has_perm(f"{meta.app_label}.manage_public_{meta.model_name}") # type: ignore[union-attr] def check_taxalist_write_permission(user, taxa_list, project) -> bool: """ - True if `user` may update/delete `taxa_list` or change its taxa. - - A public list requires the platform-wide manage_public_taxalist permission - (superusers always pass); a project-scoped list requires membership of `project`. + True if `user` may update/delete `taxa_list` or change its taxa: a public + list needs manage_public_taxalist, a project-scoped one needs membership of + `project`. Kept TaxaList-specific rather than folded into a generic helper — + ProcessingService's non-public fallback is active-staff status, not project + membership, so the two checks don't share a body. """ - if user.is_superuser: - return True if getattr(taxa_list, "is_public", False): - return user.has_perm("main.manage_public_taxalist") - return bool(project and project.members.filter(pk=user.pk).exists()) + return user_can_manage_public(user, taxa_list) + return bool(user.is_superuser or (project and project.members.filter(pk=user.pk).exists())) + + +def check_processingservice_write_permission(user, processing_service) -> bool: + """ + True if `user` may update/delete `processing_service`, or register its + pipelines: a public service needs manage_public_processingservice; a + project-scoped one only needs active-staff status, unchanged from before + is_public existed (the broader project-scoped permission model belongs to + #1120, not here). + """ + if getattr(processing_service, "is_public", False): + return user_can_manage_public(user, processing_service) + return bool(user.is_superuser or is_active_staff(user)) def add_m2m_object_permissions(user, instance, project, response_data: dict) -> dict: @@ -110,7 +129,7 @@ def add_m2m_object_permissions(user, instance, project, response_data: dict) -> Validates that the instance actually belongs to the given project before granting any permissions (prevents cross-project permission leaks). A public instance is the one exception: its update/delete permissions come from the - platform-wide manage_public_taxalist permission, not project membership. + model's manage_public_ permission, not project membership. This is a temporary approach for the M2M permission gap described in #1120. Once that issue is resolved, this should be replaced by a generic permission @@ -120,7 +139,7 @@ class (Pattern B: Bare M2M) that handles TaxaList, Taxon, ProcessingService, perms = set(response_data.get("user_permissions", [])) if getattr(instance, "is_public", False): - if user_can_manage_public_taxalist(user): + if user_can_manage_public(user, instance): perms.update(["update", "delete"]) response_data["user_permissions"] = list(perms) return response_data @@ -175,10 +194,10 @@ def has_permission(self, request, view): class IsProjectMemberOrPublicListManager(permissions.BasePermission): """ - Like IsProjectMemberOrReadOnly, but a user holding the platform-wide - manage_public_taxalist permission also passes, regardless of project - membership — for actions that can only ever land on a public list (adding - or removing a taxon from an existing TaxaList via the nested route). + Safe methods are open to everyone. Unsafe methods need membership of the + active project, or the manage_public_taxalist permission — used by the + nested add/remove-taxon route, which serves both public and project-scoped + lists and has no object to check yet at has_permission() time. """ def has_permission(self, request, view): @@ -188,7 +207,9 @@ def has_permission(self, request, view): if not request.user or not request.user.is_authenticated: return False - if request.user.is_superuser or user_can_manage_public_taxalist(request.user): # type: ignore[union-attr] + from ami.main.models import TaxaList + + if request.user.is_superuser or user_can_manage_public(request.user, TaxaList): # type: ignore[union-attr] return True get_active_project = getattr(view, "get_active_project", None) @@ -204,12 +225,11 @@ def has_permission(self, request, view): class IsProjectMemberOrPublicListManagerOrReadOnly(IsProjectMemberOrPublicListManager): """ - For TaxaListViewSet: creating a brand-new list is always project-scoped and - needs real project membership (there's no object yet to tell whether it will - be public). Update/delete defer to the object-level check below, which grants - the manage_public_taxalist bypass only once the target list's is_public flag - is known — closing the gap where a project member could edit a public list - just by supplying their own project_id. + For TaxaListViewSet: creating a brand-new list always needs real project + membership (there's no object yet to tell whether it will be public). + Update/delete defer to check_taxalist_write_permission, which grants the + manage_public_taxalist bypass only once the target list's is_public flag + is known. """ def has_permission(self, request, view): @@ -224,6 +244,49 @@ def has_object_permission(self, request, view, obj): return check_taxalist_write_permission(request.user, obj, project) +class IsActiveStaffOrPublicManager(permissions.BasePermission): + """ + Safe methods are open to everyone. Unsafe methods need active-staff status, + or the manage_public_processingservice permission — used where there is no + object yet to check at has_permission() time. + """ + + def has_permission(self, request, view): + if request.method in permissions.SAFE_METHODS: + return True + + if not request.user or not request.user.is_authenticated: + return False + + from ami.ml.models.processing_service import ProcessingService + + if request.user.is_superuser or user_can_manage_public(request.user, ProcessingService): # type: ignore + return True + + return is_active_staff(request.user) # type: ignore[arg-type] + + +class IsActiveStaffOrPublicManagerOrReadOnly(IsActiveStaffOrPublicManager): + """ + For ProcessingServiceViewSet: creating a brand-new service always needs + active-staff status (there's no object yet to tell whether it will be + public). Update/delete/register_pipelines defer to + check_processingservice_write_permission, which grants the + manage_public_processingservice bypass only once the target's is_public + flag is known. + """ + + def has_permission(self, request, view): + if getattr(view, "action", None) == "create": + return IsActiveStaffOrReadOnly.has_permission(self, request, view) + return super().has_permission(request, view) + + def has_object_permission(self, request, view, obj): + if request.method in permissions.SAFE_METHODS: + return True + return check_processingservice_write_permission(request.user, obj) + + class ObjectPermission(permissions.BasePermission): """ Generic permission class that delegates to the model's `check_permission(user, action)` method. From 058bddb7b58feeb2ab7ef045608caae52c3f6cff Mon Sep 17 00:00:00 2001 From: Michael Bunsen Date: Thu, 17 Sep 2026 14:36:31 -0700 Subject: [PATCH 07/15] feat(ml): give ProcessingService the same public/project-scoped distinction as TaxaList MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ProcessingService gets is_public and manage_public_processingservice, the same shape TaxaList already has: ProcessingServiceViewSet.get_queryset() uses for_project(project, include_public=...) instead of a plain projects=project filter, so a public service shows up for every project. Writes go through IsActiveStaffOrPublicManagerOrReadOnly — a project-scoped service keeps its existing staff-only rule unchanged (still no membership check, see #1120), but a public one now needs the platform permission; plain staff status is not enough. status and register_pipelines call self.get_object() instead of a raw ProcessingService.objects.get(pk=pk), so they inherit the same visibility and permission checks as every other detail action instead of bypassing them. Factored the ?include_public query-param parsing (shared with TaxaListViewSet) into ProjectMixin.get_include_public(), and the OpenApiParameter doc for it into include_public_doc_param next to the other shared doc params, instead of duplicating both per viewset. is_public is read-only in ProcessingServiceSerializer. Its get_permissions() now calls add_m2m_object_permissions() (previously it used the DefaultSerializer fallback, which reports empty user_permissions for a public service to anyone but a superuser, whether or not they hold the new permission). Migration 0029 is schema-only: unlike TaxaList, an existing zero-project service does NOT become public — it stays visible to superusers only until someone opts it in. Other call sites filter processing services by project directly and would wrongly exclude a public one; left unchanged, per scope, but worth knowing about: - ami/ml/models/pipeline.py:1287 (pick the lowest-latency service for a project+pipeline during job dispatch) - ami/jobs/tasks.py:74 and :189 (mark an async service as seen; count available async workers for a job) - ami/ml/views.py:116 and :246 (prefetch a pipeline's processing services for PipelineViewSet and ProjectPipelineViewSet) - ami/main/models.py:265 (project.processing_services.exists() decides whether to auto-create a project's default service) Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01TngW6AshkUEhNp9D6z3U9Z --- ami/base/views.py | 14 ++++++++ ami/main/api/schemas.py | 8 +++++ ami/main/api/views.py | 22 ++---------- ami/ml/admin.py | 2 ++ .../0029_processingservice_is_public.py | 35 +++++++++++++++++++ ami/ml/models/processing_service.py | 5 +++ ami/ml/serializers.py | 9 +++++ ami/ml/views.py | 17 ++++----- 8 files changed, 85 insertions(+), 27 deletions(-) create mode 100644 ami/ml/migrations/0029_processingservice_is_public.py diff --git a/ami/base/views.py b/ami/base/views.py index 482496b38..79631136a 100644 --- a/ami/base/views.py +++ b/ami/base/views.py @@ -94,3 +94,17 @@ def get_active_project(self) -> Project | None: raise Http404("Project not found.") return project + + def get_include_public(self) -> bool: + """ + The ?include_public query param: whether to include public rows alongside + a model's own project-scoped ones. Defaults to true; an invalid value + raises ValidationError (400) via SingleParamSerializer. + """ + from ami.base.serializers import SingleParamSerializer + + return SingleParamSerializer[bool].clean( + param_name="include_public", + field=serializers.BooleanField(required=False, default=True), + data=self.request.query_params, + ) diff --git a/ami/main/api/schemas.py b/ami/main/api/schemas.py index c9835b2fb..5b18dfb4b 100644 --- a/ami/main/api/schemas.py +++ b/ami/main/api/schemas.py @@ -13,3 +13,11 @@ required=False, type=int, ) + +include_public_doc_param = OpenApiParameter( + name="include_public", + description="Include rows that are public (available to every project), not just the ones " + "belonging to this project. Defaults to true.", + required=False, + type=bool, +) diff --git a/ami/main/api/views.py b/ami/main/api/views.py index 09f1abb44..37fd6d1b0 100644 --- a/ami/main/api/views.py +++ b/ami/main/api/views.py @@ -38,7 +38,7 @@ ) from ami.base.serializers import FilterParamsSerializer, SingleParamSerializer from ami.base.views import ProjectMixin -from ami.main.api.schemas import limit_doc_param, project_id_doc_param +from ami.main.api.schemas import include_public_doc_param, limit_doc_param, project_id_doc_param from ami.main.api.serializers import TagSerializer from ami.main.models_future.identifications import create_identifications_batch, resolve_occurrences from ami.main.models_future.occurrence import model_agreement_for_project, top_identifiers_for_project @@ -2225,25 +2225,9 @@ def get_queryset(self): project = self.get_active_project() if not project: return qs - include_public = SingleParamSerializer[bool].clean( - param_name="include_public", - field=serializers.BooleanField(required=False, default=True), - data=self.request.query_params, - ) - return qs.for_project(project, include_public=include_public) + return qs.for_project(project, include_public=self.get_include_public()) - @extend_schema( - parameters=[ - project_id_doc_param, - OpenApiParameter( - name="include_public", - description="Include taxa lists that are public (available to every project), " - "not just the ones belonging to this project. Defaults to true.", - required=False, - type=OpenApiTypes.BOOL, - ), - ] - ) + @extend_schema(parameters=[project_id_doc_param, include_public_doc_param]) def list(self, request, *args, **kwargs): return super().list(request, *args, **kwargs) diff --git a/ami/ml/admin.py b/ami/ml/admin.py index 008b20e84..1a8a78a89 100644 --- a/ami/ml/admin.py +++ b/ami/ml/admin.py @@ -70,8 +70,10 @@ class ProcessingServiceAdmin(AdminBase): "id", "name", "endpoint_url", + "is_public", "created_at", ] + list_filter = ["is_public"] @admin.register(AlgorithmCategoryMap) diff --git a/ami/ml/migrations/0029_processingservice_is_public.py b/ami/ml/migrations/0029_processingservice_is_public.py new file mode 100644 index 000000000..73e4626d5 --- /dev/null +++ b/ami/ml/migrations/0029_processingservice_is_public.py @@ -0,0 +1,35 @@ +""" +``ProcessingService.is_public`` marks a service as available to every project, +not just the ones in its ``projects`` M2M. Schema only: unlike TaxaList, an +existing zero-project service does not become public here — it stays visible +to superusers only until someone explicitly opts it in. +""" + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("ml", "0028_normalize_empty_endpoint_url_to_null"), + ] + + operations = [ + migrations.AlterModelOptions( + name="processingservice", + options={ + "permissions": [ + ("manage_public_processingservice", "Can create, edit and delete public processing services") + ], + "verbose_name": "Processing Service", + "verbose_name_plural": "Processing Services", + }, + ), + migrations.AddField( + model_name="processingservice", + name="is_public", + field=models.BooleanField( + default=False, + help_text="Public processing services are available to every project, not just the ones in 'projects'.", + ), + ), + ] diff --git a/ami/ml/models/processing_service.py b/ami/ml/models/processing_service.py index fce1aefc5..c781f9f1d 100644 --- a/ami/ml/models/processing_service.py +++ b/ami/ml/models/processing_service.py @@ -70,6 +70,10 @@ class ProcessingService(BaseModel): last_seen = models.DateTimeField(null=True) last_seen_live = models.BooleanField(null=True) last_seen_latency = models.FloatField(null=True) + is_public = models.BooleanField( + default=False, + help_text="Public processing services are available to every project, not just the ones in 'projects'.", + ) objects = ProcessingServiceManager() @@ -89,6 +93,7 @@ def __str__(self): class Meta: verbose_name = "Processing Service" verbose_name_plural = "Processing Services" + permissions = [("manage_public_processingservice", "Can create, edit and delete public processing services")] def create_pipelines( self, diff --git a/ami/ml/serializers.py b/ami/ml/serializers.py index e7e9e6aaf..5f833d24a 100644 --- a/ami/ml/serializers.py +++ b/ami/ml/serializers.py @@ -1,6 +1,8 @@ from django_pydantic_field.rest_framework import SchemaField from rest_framework import serializers +from ami.base.permissions import add_m2m_object_permissions +from ami.base.views import get_active_project from ami.main.api.serializers import DefaultSerializer, MinimalNestedModelSerializer from .models.algorithm import Algorithm, AlgorithmCategoryMap @@ -138,6 +140,7 @@ class ProcessingServiceSerializer(DefaultSerializer): pipelines = PipelineNestedSerializer(many=True, read_only=True) projects = serializers.SerializerMethodField() is_async = serializers.BooleanField(read_only=True) + is_public = serializers.BooleanField(read_only=True) endpoint_url = serializers.CharField(required=False, allow_null=True, allow_blank=False, max_length=1024) class Meta: @@ -150,6 +153,7 @@ class Meta: "projects", "endpoint_url", "is_async", + "is_public", "pipelines", "created_at", "updated_at", @@ -164,6 +168,11 @@ def get_projects(self, obj): """ return list(obj.projects.values_list("id", flat=True)) + def get_permissions(self, instance, instance_data): + request = self.context["request"] + project = get_active_project(request=request) + return add_m2m_object_permissions(request.user, instance, project, instance_data) + class PipelineRegistrationSerializer(serializers.Serializer): processing_service_name = serializers.CharField() diff --git a/ami/ml/views.py b/ami/ml/views.py index 63e460af6..df66c02a4 100644 --- a/ami/ml/views.py +++ b/ami/ml/views.py @@ -11,9 +11,9 @@ from rest_framework.request import Request from rest_framework.response import Response -from ami.base.permissions import ProjectPipelineConfigPermission +from ami.base.permissions import IsActiveStaffOrPublicManagerOrReadOnly, ProjectPipelineConfigPermission from ami.base.views import ProjectMixin -from ami.main.api.schemas import project_id_doc_param +from ami.main.api.schemas import include_public_doc_param, project_id_doc_param from ami.main.api.views import DefaultViewSet from ami.main.models import Project, SourceImage from ami.ml.schemas import PipelineRegistrationResponse @@ -168,16 +168,17 @@ class ProcessingServiceViewSet(DefaultViewSet, ProjectMixin): serializer_class = ProcessingServiceSerializer filterset_fields = ["projects"] ordering_fields = ["id", "created_at", "updated_at"] + permission_classes = [IsActiveStaffOrPublicManagerOrReadOnly] require_project = True def get_queryset(self) -> QuerySet: qs: QuerySet = super().get_queryset() project = self.get_active_project() - if project: - qs = qs.filter(projects=project) - return qs + if not project: + return qs + return qs.for_project(project, include_public=self.get_include_public()) - @extend_schema(parameters=[project_id_doc_param]) + @extend_schema(parameters=[project_id_doc_param, include_public_doc_param]) def list(self, request, *args, **kwargs): return super().list(request, *args, **kwargs) @@ -214,13 +215,13 @@ def status(self, request: Request, pk=None) -> Response: """ Test the connection to the processing service. """ - processing_service = ProcessingService.objects.get(pk=pk) + processing_service = self.get_object() response = processing_service.get_status() return Response(response.dict()) @action(detail=True, methods=["post"]) def register_pipelines(self, request: Request, pk=None) -> Response: - processing_service = ProcessingService.objects.get(pk=pk) + processing_service = self.get_object() response = processing_service.create_pipelines() processing_service.save() return Response(response.dict()) From e81d557665d8debbf4d4d64dbe6b673a2c0eea4d Mon Sep 17 00:00:00 2001 From: Michael Bunsen Date: Thu, 17 Sep 2026 14:36:43 -0700 Subject: [PATCH 08/15] =?UTF-8?q?test(ml):=20cover=20public=20processing?= =?UTF-8?q?=20services=20=E2=80=94=20permissions,=20visibility,=20and=20qu?= =?UTF-8?q?ery=20cost?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the permission matrix for public vs. project-scoped ProcessingServices (staff, member, non-member, anonymous, superuser, and a staff user granted manage_public_processingservice) across update, delete, status, and register_pipelines; the include_public query param and its 400 on an invalid value; a public service linked to several projects appearing once; a non-public service with zero projects being invisible to non-superusers; and a multi-row assertNumQueries check on the list endpoint. All fixtures use endpoint_url=None so get_status()/create_pipelines() never make a real network call, matching the existing pull-mode test pattern in this file. Fixes _register_pipelines()'s test helper, which never passed project_id: that worked before only because the action did a raw pk lookup bypassing get_queryset() entirely; now that it calls self.get_object(), the same project_id ProcessingServiceViewSet's other actions already require applies here too. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01TngW6AshkUEhNp9D6z3U9Z --- ami/ml/tests.py | 263 +++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 260 insertions(+), 3 deletions(-) diff --git a/ami/ml/tests.py b/ami/ml/tests.py index bd92bb02f..63c48158a 100644 --- a/ami/ml/tests.py +++ b/ami/ml/tests.py @@ -4,8 +4,12 @@ import unittest import uuid -from django.test import TestCase -from rest_framework.test import APIRequestFactory, APITestCase +from django.contrib.auth.models import Permission +from django.db import connection +from django.test import TestCase, override_settings +from django.test.utils import CaptureQueriesContext +from rest_framework import status +from rest_framework.test import APIClient, APIRequestFactory, APITestCase from ami.base.serializers import reverse_with_params from ami.main.models import ( @@ -84,7 +88,9 @@ def _delete_processing_service(self, processing_service_id: int): def _register_pipelines(self, processing_service_id): processing_services_register_pipelines_url = reverse_with_params( - "api:processingservice-register-pipelines", args=[processing_service_id] + "api:processingservice-register-pipelines", + args=[processing_service_id], + params={"project_id": self.project.pk}, ) self.client.force_authenticate(user=self.user) resp = self.client.post(processing_services_register_pipelines_url) @@ -227,6 +233,257 @@ def test_model_has_last_seen_fields(self): self.assertFalse(hasattr(service, "last_checked_latency")) +class ProcessingServicePublicPermissionsTestCase(TestCase): + """ + Permission matrix for public vs. project-scoped ProcessingServices. + + A project-scoped service keeps its existing staff-only write rule (any + active staff member, project membership not required). A public service + instead requires the manage_public_processingservice platform permission + (or a superuser) — plain staff status is not enough. All services use + endpoint_url=None (pull-mode) so get_status()/create_pipelines() never + make a real network call. + """ + + def setUp(self): + self.staff = User.objects.create_user(email="staff-ps@example.com", password="testpass", is_staff=True) + self.member = User.objects.create_user(email="member-ps@example.com", password="testpass") + self.non_member = User.objects.create_user(email="nonmember-ps@example.com", password="testpass") + self.superuser = User.objects.create_superuser(email="super-ps@example.com", password="testpass") + self.public_manager = User.objects.create_user( + email="manager-ps@example.com", password="testpass", is_staff=True + ) + perm = Permission.objects.get(codename="manage_public_processingservice", content_type__app_label="ml") + self.public_manager.user_permissions.add(perm) + + self.project = Project.objects.create(name="PS Test Project", create_defaults=False) + self.project.members.add(self.member) + + self.public_service = ProcessingService.objects.create( + name="Public Service", endpoint_url=None, is_public=True + ) + self.scoped_service = ProcessingService.objects.create(name="Scoped Service", endpoint_url=None) + self.scoped_service.projects.add(self.project) + + self.client = APIClient() + + def _detail_url(self, service): + return f"/api/v2/ml/processing_services/{service.pk}/?project_id={self.project.pk}" + + def _status_url(self, service): + return f"/api/v2/ml/processing_services/{service.pk}/status/?project_id={self.project.pk}" + + def _register_url(self, service): + return f"/api/v2/ml/processing_services/{service.pk}/register_pipelines/?project_id={self.project.pk}" + + # -- Update -- + + def test_staff_can_update_scoped_service(self): + self.client.force_authenticate(self.staff) + response = self.client.patch(self._detail_url(self.scoped_service), {"name": "Renamed"}) + self.assertEqual(response.status_code, status.HTTP_200_OK) + + def test_staff_cannot_update_public_service(self): + """Plain staff status is not enough for a public service.""" + self.client.force_authenticate(self.staff) + response = self.client.patch(self._detail_url(self.public_service), {"name": "Hacked"}) + self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) + + def test_member_cannot_update_scoped_service(self): + """Project membership alone is not the write gate here — staff status is.""" + self.client.force_authenticate(self.member) + response = self.client.patch(self._detail_url(self.scoped_service), {"name": "Hacked"}) + self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) + + def test_non_member_cannot_update_scoped_service(self): + self.client.force_authenticate(self.non_member) + response = self.client.patch(self._detail_url(self.scoped_service), {"name": "Hacked"}) + self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) + + def test_anonymous_cannot_update_scoped_service(self): + response = self.client.patch(self._detail_url(self.scoped_service), {"name": "Hacked"}) + self.assertIn(response.status_code, [status.HTTP_401_UNAUTHORIZED, status.HTTP_403_FORBIDDEN]) + + def test_superuser_can_update_public_service(self): + self.client.force_authenticate(self.superuser) + response = self.client.patch(self._detail_url(self.public_service), {"name": "Renamed by super"}) + self.assertEqual(response.status_code, status.HTTP_200_OK) + + def test_public_manager_can_update_public_service(self): + self.client.force_authenticate(self.public_manager) + response = self.client.patch(self._detail_url(self.public_service), {"name": "Renamed"}) + self.assertEqual(response.status_code, status.HTTP_200_OK) + + def test_public_manager_can_still_update_scoped_service(self): + """The public manager is also staff, so the existing staff-only rule still applies.""" + self.client.force_authenticate(self.public_manager) + response = self.client.patch(self._detail_url(self.scoped_service), {"name": "Renamed"}) + self.assertEqual(response.status_code, status.HTTP_200_OK) + + # -- Delete -- + + def test_staff_cannot_delete_public_service(self): + self.client.force_authenticate(self.staff) + response = self.client.delete(self._detail_url(self.public_service)) + self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) + + def test_staff_can_delete_scoped_service(self): + self.client.force_authenticate(self.staff) + response = self.client.delete(self._detail_url(self.scoped_service)) + self.assertEqual(response.status_code, status.HTTP_204_NO_CONTENT) + + def test_public_manager_can_delete_public_service(self): + self.client.force_authenticate(self.public_manager) + response = self.client.delete(self._detail_url(self.public_service)) + self.assertEqual(response.status_code, status.HTTP_204_NO_CONTENT) + + # -- Retrieve / list visibility -- + + def test_anonymous_can_retrieve_public_service(self): + response = self.client.get(self._detail_url(self.public_service)) + self.assertEqual(response.status_code, status.HTTP_200_OK) + + def test_non_member_can_see_public_service(self): + self.client.force_authenticate(self.non_member) + response = self.client.get(f"/api/v2/ml/processing_services/?project_id={self.project.pk}") + self.assertEqual(response.status_code, status.HTTP_200_OK) + ids = {row["id"] for row in response.json()["results"]} + self.assertIn(self.public_service.pk, ids) + + def test_is_public_field_is_read_only_and_reported(self): + self.client.force_authenticate(self.staff) + response = self.client.get(self._detail_url(self.public_service)) + self.assertTrue(response.json()["is_public"]) + + response = self.client.patch(self._detail_url(self.scoped_service), {"is_public": True}) + self.scoped_service.refresh_from_db() + self.assertFalse(self.scoped_service.is_public) + + def test_user_permissions_include_update_delete_for_public_manager_only(self): + self.client.force_authenticate(self.staff) + response = self.client.get(self._detail_url(self.public_service)) + self.assertNotIn("update", response.json()["user_permissions"]) + + self.client.force_authenticate(self.public_manager) + response = self.client.get(self._detail_url(self.public_service)) + perms = response.json()["user_permissions"] + self.assertIn("update", perms) + self.assertIn("delete", perms) + + # -- status (a read-type action; open to everyone like any other safe method) -- + + def test_anonymous_can_check_status_of_public_service(self): + response = self.client.get(self._status_url(self.public_service)) + self.assertEqual(response.status_code, status.HTTP_200_OK) + + def test_non_member_can_check_status_of_scoped_service(self): + self.client.force_authenticate(self.non_member) + response = self.client.get(self._status_url(self.scoped_service)) + self.assertEqual(response.status_code, status.HTTP_200_OK) + + # -- register_pipelines -- + + def test_staff_cannot_register_pipelines_on_public_service(self): + self.client.force_authenticate(self.staff) + response = self.client.post(self._register_url(self.public_service)) + self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) + + def test_staff_can_register_pipelines_on_scoped_service(self): + self.client.force_authenticate(self.staff) + response = self.client.post(self._register_url(self.scoped_service)) + self.assertEqual(response.status_code, status.HTTP_200_OK) + + def test_public_manager_can_register_pipelines_on_public_service(self): + self.client.force_authenticate(self.public_manager) + response = self.client.post(self._register_url(self.public_service)) + self.assertEqual(response.status_code, status.HTTP_200_OK) + + def test_member_cannot_register_pipelines_on_scoped_service(self): + self.client.force_authenticate(self.member) + response = self.client.post(self._register_url(self.scoped_service)) + self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) + + +class ProcessingServiceIncludePublicParamTestCase(TestCase): + """?include_public toggles whether public services appear alongside a project's own.""" + + def setUp(self): + self.user = User.objects.create_user(email="ps-scope-user@example.com", password="testpass") + self.project = Project.objects.create(name="PS Scope Project", create_defaults=False) + self.project.members.add(self.user) + self.other_project = Project.objects.create(name="PS Other Scope Project", create_defaults=False) + + self.scoped_service = ProcessingService.objects.create(name="PS Scoped", endpoint_url=None) + self.scoped_service.projects.add(self.project) + self.public_service = ProcessingService.objects.create(name="PS Public", endpoint_url=None, is_public=True) + # A public service can also be linked to an unrelated project without appearing twice. + self.public_service.projects.add(self.other_project) + + self.hidden_service = ProcessingService.objects.create(name="PS Hidden with no project", endpoint_url=None) + + self.client = APIClient() + self.client.force_authenticate(self.user) + + def _list_ids(self, **params): + params["project_id"] = self.project.pk + query = "&".join(f"{k}={v}" for k, v in params.items()) + response = self.client.get(f"/api/v2/ml/processing_services/?{query}") + self.assertEqual(response.status_code, status.HTTP_200_OK) + return response.json()["results"] + + def test_public_services_included_by_default(self): + ids = {row["id"] for row in self._list_ids()} + self.assertEqual(ids, {self.scoped_service.pk, self.public_service.pk}) + + def test_include_public_false_hides_public_services(self): + ids = {row["id"] for row in self._list_ids(include_public="false")} + self.assertEqual(ids, {self.scoped_service.pk}) + + def test_include_public_invalid_value_returns_400(self): + response = self.client.get( + f"/api/v2/ml/processing_services/?project_id={self.project.pk}&include_public=notabool" + ) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + + def test_public_service_linked_to_multiple_projects_appears_once(self): + """A public service attached to several projects must not be duplicated by the M2M join.""" + self.public_service.projects.add(self.project) + rows = self._list_ids() + matches = [row for row in rows if row["id"] == self.public_service.pk] + self.assertEqual(len(matches), 1) + + def test_hidden_zero_project_service_is_invisible_to_non_superuser(self): + ids = {row["id"] for row in self._list_ids()} + self.assertNotIn(self.hidden_service.pk, ids) + + +@override_settings(CACHALOT_ENABLED=False) +class ProcessingServiceQueryCountTestCase(APITestCase): + """Audit ProcessingServiceViewSet.list for N+1 across a mixed public/scoped, multi-row fixture.""" + + def setUp(self): + self.user = User.objects.create_user(email="ps-qc-user@example.com", password="testpass") + self.project = Project.objects.create(name="PS QC Project", create_defaults=False) + self.project.members.add(self.user) + for i in range(3): + scoped = ProcessingService.objects.create(name=f"PS Scoped {i}", endpoint_url=None) + scoped.projects.add(self.project) + for i in range(2): + ProcessingService.objects.create(name=f"PS Public {i}", endpoint_url=None, is_public=True) + self.client = APIClient() + self.client.force_authenticate(self.user) + + def test_list_query_count(self): + from cachalot.api import cachalot_disabled + + url = f"/api/v2/ml/processing_services/?project_id={self.project.pk}" + with cachalot_disabled(), CaptureQueriesContext(connection) as ctx: + response = self.client.get(url) + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(len(response.json()["results"]), 5) + self.assertEqual(len(ctx.captured_queries), 34) + + class TestProjectPipelineRegistrationUpdatesLastSeen(APITestCase): """Test that async pipeline registration updates last_seen on the processing service.""" From 92bc2ac6c410f8dd50f29ae21bab22462bba44fe Mon Sep 17 00:00:00 2001 From: Michael Bunsen Date: Thu, 17 Sep 2026 14:52:25 -0700 Subject: [PATCH 09/15] fix(ml): make project_id optional on status/register_pipelines to match the frontend The frontend calls both endpoints without a project_id (usePopulateProcessingService.ts, useTestProcessingServiceConnection.ts), a call shape the previous raw ProcessingService.objects.get(pk=pk) lookup tolerated because it never checked project_id at all. Switching those actions to self.get_object() picked up require_project=True from the rest of the viewset, so both buttons started returning 400. ProcessingServiceViewSet.get_active_project() now passes required=False for exactly these two actions; get_queryset() already falls back to the plain visible_for_user() set when no project is given, so the two actions resolve against every service the user can see instead of one project's services. Every other action (list, create, retrieve, update, destroy) still requires project_id, unchanged. Restored the pre-existing _register_pipelines test helper to its original call shape (no project_id) and added a with_project_id=True variant, plus the same pair for status and a public-service permission check confirming a staff user without manage_public_processingservice is denied register_pipelines under both call shapes. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01TngW6AshkUEhNp9D6z3U9Z --- ami/ml/tests.py | 65 ++++++++++++++++++++++++++++++++++++++++++++++--- ami/ml/views.py | 11 ++++++++- 2 files changed, 71 insertions(+), 5 deletions(-) diff --git a/ami/ml/tests.py b/ami/ml/tests.py index 63c48158a..b0251e6fc 100644 --- a/ami/ml/tests.py +++ b/ami/ml/tests.py @@ -86,11 +86,14 @@ def _delete_processing_service(self, processing_service_id: int): self.assertEqual(resp.status_code, 204) return resp - def _register_pipelines(self, processing_service_id): + def _register_pipelines(self, processing_service_id, with_project_id=False): + """ + Pins the frontend's call shape: usePopulateProcessingService.ts POSTs this + endpoint with no project_id. with_project_id=True exercises the other shape. + """ + params = {"project_id": self.project.pk} if with_project_id else {} processing_services_register_pipelines_url = reverse_with_params( - "api:processingservice-register-pipelines", - args=[processing_service_id], - params={"project_id": self.project.pk}, + "api:processingservice-register-pipelines", args=[processing_service_id], params=params ) self.client.force_authenticate(user=self.user) resp = self.client.post(processing_services_register_pipelines_url) @@ -114,6 +117,8 @@ def test_project_was_added(self): self.assertIn(self.project, processing_service.projects.all()) def test_processing_service_pipeline_registration(self): + """Pins the frontend's call shape: usePopulateProcessingService.ts POSTs register_pipelines + with no project_id, relying on the endpoint resolving the user-visible set instead.""" # register a processing service response = self._create_processing_service( name="Processing Service Test", @@ -128,6 +133,43 @@ def test_processing_service_pipeline_registration(self): self.assertEqual(pipelines_queryset.count(), len(response["pipelines"])) + def test_processing_service_pipeline_registration_with_project_id(self): + """The other call shape: register_pipelines also works when project_id is supplied.""" + response = self._create_processing_service( + name="Processing Service Test With Project", endpoint_url="http://processing_service:2000" + ) + processing_service_id = response["id"] + + response = self._register_pipelines(processing_service_id, with_project_id=True) + processing_service = ProcessingService.objects.get(pk=processing_service_id) + + self.assertEqual(processing_service.pipelines.count(), len(response["pipelines"])) + + def test_check_status_without_project_id(self): + """Pins the frontend's call shape: useTestProcessingServiceConnection.ts GETs status + with no project_id.""" + service = ProcessingService.objects.create(name="Status Check Service", endpoint_url=None) + service.projects.add(self.project) + url = reverse_with_params("api:processingservice-status", args=[service.pk]) + + self.client.force_authenticate(user=self.user) + response = self.client.get(url) + + self.assertEqual(response.status_code, 200) + + def test_check_status_with_project_id(self): + """The other call shape: status also works when project_id is supplied.""" + service = ProcessingService.objects.create(name="Status Check Service With Project", endpoint_url=None) + service.projects.add(self.project) + url = reverse_with_params( + "api:processingservice-status", args=[service.pk], params={"project_id": self.project.pk} + ) + + self.client.force_authenticate(user=self.user) + response = self.client.get(url) + + self.assertEqual(response.status_code, 200) + def test_create_processing_service_without_endpoint_url(self): """Test creating a ProcessingService without endpoint_url (pull mode)""" processing_services_create_url = reverse_with_params( @@ -276,6 +318,10 @@ def _status_url(self, service): def _register_url(self, service): return f"/api/v2/ml/processing_services/{service.pk}/register_pipelines/?project_id={self.project.pk}" + def _register_url_no_project(self, service): + """Pins the frontend's call shape: usePopulateProcessingService.ts POSTs with no project_id.""" + return f"/api/v2/ml/processing_services/{service.pk}/register_pipelines/" + # -- Update -- def test_staff_can_update_scoped_service(self): @@ -403,6 +449,17 @@ def test_member_cannot_register_pipelines_on_scoped_service(self): response = self.client.post(self._register_url(self.scoped_service)) self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) + def test_staff_cannot_register_pipelines_on_public_service_without_project_id(self): + """The platform-permission gate applies the same whether or not project_id is supplied.""" + self.client.force_authenticate(self.staff) + response = self.client.post(self._register_url_no_project(self.public_service)) + self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) + + def test_public_manager_can_register_pipelines_on_public_service_without_project_id(self): + self.client.force_authenticate(self.public_manager) + response = self.client.post(self._register_url_no_project(self.public_service)) + self.assertEqual(response.status_code, status.HTTP_200_OK) + class ProcessingServiceIncludePublicParamTestCase(TestCase): """?include_public toggles whether public services appear alongside a project's own.""" diff --git a/ami/ml/views.py b/ami/ml/views.py index df66c02a4..c24456762 100644 --- a/ami/ml/views.py +++ b/ami/ml/views.py @@ -12,7 +12,7 @@ from rest_framework.response import Response from ami.base.permissions import IsActiveStaffOrPublicManagerOrReadOnly, ProjectPipelineConfigPermission -from ami.base.views import ProjectMixin +from ami.base.views import ProjectMixin, get_active_project from ami.main.api.schemas import include_public_doc_param, project_id_doc_param from ami.main.api.views import DefaultViewSet from ami.main.models import Project, SourceImage @@ -171,6 +171,15 @@ class ProcessingServiceViewSet(DefaultViewSet, ProjectMixin): permission_classes = [IsActiveStaffOrPublicManagerOrReadOnly] require_project = True + def get_active_project(self) -> Project | None: + """ + project_id is optional for status/register_pipelines — the frontend calls both + without one — but stays required for every other action, as declared above. + """ + if self.action in ("status", "register_pipelines"): + return get_active_project(request=self.request, kwargs=self.kwargs, required=False) + return super().get_active_project() + def get_queryset(self) -> QuerySet: qs: QuerySet = super().get_queryset() project = self.get_active_project() From 2e6ee8f7d0a5a2f04431f816b025d7514d6c7d22 Mon Sep 17 00:00:00 2001 From: Michael Bunsen Date: Fri, 18 Sep 2026 07:06:41 -0700 Subject: [PATCH 10/15] fix(taxa+ml): scope include_public to list, stop a draft-project-id leak, cut ProcessingService permission cost MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit include_public governed every action's queryset, not just list: a public list/service retrieved, updated, or deleted with ?include_public=false 404'd instead of resolving normally, since for_project() dropped it from the candidate set entirely. TaxaListViewSet and ProcessingServiceViewSet now only apply it when self.action == "list"; every other action always includes public rows. TaxaListSerializer.get_projects() and ProcessingServiceSerializer.get_projects() emitted every linked project id unconditionally. Since a public row bypasses the draft-project visibility filter, an outsider retrieving a public list or service linked to a draft project could learn that project's id even though they can't see the project itself. Both now intersect with Project.objects.visible_for_user(request.user), computed once per request (cached on the serializer instance, the same request-scoped-cache-on-self shape used elsewhere for this exact reason) and reused across every row a ListSerializer renders. ProcessingServiceSerializer.get_permissions() called add_m2m_object_permissions, which ran a membership .exists() query and a guardian get_perms() lookup for every non-public row — pure overhead, since no per-project *_processingservice guardian permission exists anywhere in this codebase (checked Project.Permissions and ami/users/roles.py), so that branch could only ever grant anything to a superuser, which a plain attribute check already covers. New add_processingservice_permissions() keeps only the public-row platform-permission check and the superuser check. get_or_create_for_project(project=X, is_public=True) silently dropped is_public instead of raising: a project-scoped list can't also be the platform's public list of that name, and silently ignoring the caller's is_public=True hid a mistake instead of surfacing it. Raises ValueError now. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01TngW6AshkUEhNp9D6z3U9Z --- ami/base/permissions.py | 21 +++++++++++++++++++++ ami/main/api/serializers.py | 14 +++++++++++--- ami/main/api/views.py | 6 +++++- ami/main/models.py | 20 +++++++++++--------- ami/ml/serializers.py | 22 ++++++++++++++-------- ami/ml/views.py | 6 +++++- 6 files changed, 67 insertions(+), 22 deletions(-) diff --git a/ami/base/permissions.py b/ami/base/permissions.py index 02c54d89d..0f0dfc764 100644 --- a/ami/base/permissions.py +++ b/ami/base/permissions.py @@ -117,6 +117,27 @@ def check_processingservice_write_permission(user, processing_service) -> bool: return bool(user.is_superuser or is_active_staff(user)) +def add_processingservice_permissions(user, instance, response_data: dict) -> dict: + """ + Add update/delete to user_permissions for a ProcessingService. + + Unlike add_m2m_object_permissions, this skips the M2M membership check and + the guardian lookup entirely: no per-project *_processingservice guardian + permission exists anywhere in this codebase (see Project.Permissions and + ami/users/roles.py), so that branch could only ever fire for a superuser, + which a plain attribute check already covers for free. A public instance + still checks the platform manage_public_processingservice permission. + """ + perms = set(response_data.get("user_permissions", [])) + if getattr(instance, "is_public", False): + if user_can_manage_public(user, instance): + perms.update(["update", "delete"]) + elif user.is_superuser: + perms.update(["update", "delete"]) + response_data["user_permissions"] = list(perms) + return response_data + + def add_m2m_object_permissions(user, instance, project, response_data: dict) -> dict: """ Add object-level permissions for models with an M2M relationship to Project. diff --git a/ami/main/api/serializers.py b/ami/main/api/serializers.py index c18cb395a..923bbe2a6 100644 --- a/ami/main/api/serializers.py +++ b/ami/main/api/serializers.py @@ -748,10 +748,18 @@ def get_permissions(self, instance, instance_data): def get_projects(self, obj): """ - Return list of project IDs this taxa list belongs to. - This is read-only and managed by the server. + Return the ids of this list's linked projects that are visible to the + requester. A public list can be linked to a draft project it's otherwise + not visible in; without this filter, an outsider retrieving the public + list would learn that draft project's id even though they can't see the + project itself. """ - return list(obj.projects.values_list("id", flat=True)) + request = self.context["request"] + if not hasattr(self, "_visible_project_ids"): + self._visible_project_ids = set( + Project.objects.visible_for_user(request.user).values_list("id", flat=True) + ) + return [pid for pid in obj.projects.values_list("id", flat=True) if pid in self._visible_project_ids] class TaxaListTaxonInputSerializer(serializers.Serializer): diff --git a/ami/main/api/views.py b/ami/main/api/views.py index 37fd6d1b0..22a69acbc 100644 --- a/ami/main/api/views.py +++ b/ami/main/api/views.py @@ -2225,7 +2225,11 @@ def get_queryset(self): project = self.get_active_project() if not project: return qs - return qs.for_project(project, include_public=self.get_include_public()) + # include_public governs the list action's default scope, not whether a + # specific row is reachable: a detail/update/delete on a public list must + # still resolve it even under ?include_public=false. + include_public = self.get_include_public() if self.action == "list" else True + return qs.for_project(project, include_public=include_public) @extend_schema(parameters=[project_id_doc_param, include_public_doc_param]) def list(self, request, *args, **kwargs): diff --git a/ami/main/models.py b/ami/main/models.py index f3112ef44..7491ab425 100644 --- a/ami/main/models.py +++ b/ami/main/models.py @@ -34,7 +34,7 @@ import ami.tasks import ami.utils from ami.base.fields import DateStringField -from ami.base.models import BaseModel, BaseQuerySet +from ami.base.models import BaseModel, BaseQuerySet, PublicScopedModel from ami.main import charts from ami.main.models_future.filters import ( build_occurrence_default_filters_q, @@ -4734,14 +4734,15 @@ def get_or_create_for_project( """ Get or create a TaxaList with uniqueness scoped to project. - - project is given: looks for/creates a list associated with that project (``is_public`` is ignored) + - project is given: looks for/creates a list associated with that project - project is None, is_public=True: looks for/creates the public list with this name - project is None, is_public=False: looks for/creates a hidden list with no project :param name: Name of the taxa list. :param project: Project to scope the list to, or None for a list with no project. :param is_public: When ``project`` is None, whether the list is the public list of this - name or a hidden one (e.g. a per-algorithm category-map list). Ignored otherwise. + name or a hidden one (e.g. a per-algorithm category-map list). Raises when combined + with a ``project``, since a project-scoped list can't also be a public one. :param defaults: Extra field values applied only when creating a new list (ignored on the get path, matching Django's ``get_or_create`` semantics). @@ -4752,6 +4753,9 @@ def get_or_create_for_project( Returns: Tuple of (TaxaList, created: bool) """ + if project is not None and is_public: + raise ValueError("get_or_create_for_project() cannot create a public list scoped to a single project.") + if project is not None: # Project-specific: find list with this name in this project qs = self.filter(name=name, projects=project) @@ -4768,7 +4772,9 @@ def get_or_create_for_project( return qs.get(), False except self.model.DoesNotExist: with transaction.atomic(): - taxa_list = self.create(name=name, is_public=is_public if project is None else False, **defaults) + # is_public is guaranteed False here whenever project is set — the guard above + # already rejects the combination that would make this ambiguous. + taxa_list = self.create(name=name, is_public=is_public, **defaults) if project: taxa_list.projects.add(project) return taxa_list, True @@ -4784,7 +4790,7 @@ class TaxaListManager(models.Manager.from_queryset(TaxaListQuerySet)): @final -class TaxaList(BaseModel): +class TaxaList(BaseModel, PublicScopedModel): """A checklist of taxa""" name = models.CharField(max_length=255) @@ -4792,10 +4798,6 @@ class TaxaList(BaseModel): taxa = models.ManyToManyField(Taxon, related_name="lists") projects = models.ManyToManyField("Project", related_name="taxa_lists", blank=True) - is_public = models.BooleanField( - default=False, - help_text="Public lists are available to every project, not just the ones in 'projects'.", - ) objects: TaxaListManager = TaxaListManager() diff --git a/ami/ml/serializers.py b/ami/ml/serializers.py index 5f833d24a..57876a211 100644 --- a/ami/ml/serializers.py +++ b/ami/ml/serializers.py @@ -1,9 +1,9 @@ from django_pydantic_field.rest_framework import SchemaField from rest_framework import serializers -from ami.base.permissions import add_m2m_object_permissions -from ami.base.views import get_active_project +from ami.base.permissions import add_processingservice_permissions from ami.main.api.serializers import DefaultSerializer, MinimalNestedModelSerializer +from ami.main.models import Project from .models.algorithm import Algorithm, AlgorithmCategoryMap from .models.pipeline import Pipeline, PipelineStage @@ -163,15 +163,21 @@ class Meta: def get_projects(self, obj): """ - Return list of project IDs this processing service belongs to. - This is read-only and managed by the server. + Return the ids of this service's linked projects that are visible to the + requester. A public service can be linked to a draft project it's + otherwise not visible in; without this filter, an outsider retrieving the + public service would learn that draft project's id even though they + can't see the project itself. """ - return list(obj.projects.values_list("id", flat=True)) + request = self.context["request"] + if not hasattr(self, "_visible_project_ids"): + self._visible_project_ids = set( + Project.objects.visible_for_user(request.user).values_list("id", flat=True) + ) + return [pid for pid in obj.projects.values_list("id", flat=True) if pid in self._visible_project_ids] def get_permissions(self, instance, instance_data): - request = self.context["request"] - project = get_active_project(request=request) - return add_m2m_object_permissions(request.user, instance, project, instance_data) + return add_processingservice_permissions(self.context["request"].user, instance, instance_data) class PipelineRegistrationSerializer(serializers.Serializer): diff --git a/ami/ml/views.py b/ami/ml/views.py index c24456762..cb3997a58 100644 --- a/ami/ml/views.py +++ b/ami/ml/views.py @@ -185,7 +185,11 @@ def get_queryset(self) -> QuerySet: project = self.get_active_project() if not project: return qs - return qs.for_project(project, include_public=self.get_include_public()) + # include_public governs the list action's default scope, not whether a + # specific row is reachable: a detail/update/delete/status/register_pipelines + # on a public service must still resolve it even under ?include_public=false. + include_public = self.get_include_public() if self.action == "list" else True + return qs.for_project(project, include_public=include_public) @extend_schema(parameters=[project_id_doc_param, include_public_doc_param]) def list(self, request, *args, **kwargs): From e750e8cd2ceb8f10f17dfb5acb191643f01d1154 Mon Sep 17 00:00:00 2001 From: Michael Bunsen Date: Fri, 18 Sep 2026 07:07:17 -0700 Subject: [PATCH 11/15] refactor(base): explicit PublicScopedModel marker instead of duck-typed is_public MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit for_project() and visible_for_user() decided their public-row bypass with hasattr(model, "is_public") — any model that later grew an attribute of that name would silently pick up public-row visibility rules it never asked for. New abstract PublicScopedModel (ami/base/models.py) declares is_public once; TaxaList and ProcessingService inherit it (ProcessingService overrides the field only to give it its own help text). Both queryset methods now check issubclass(model, PublicScopedModel) instead. Verified the refactor alone is migration-neutral (makemigrations --check: no changes) before touching wording, so drift from moving the field would have been caught separately from the wording fix below. Reworded the field's help text in the same change, since both needed the same scrutiny: TaxaList's "available to every project" already held (no other code path filters TaxaList by project bypassing is_public); a job still won't dispatch to a public ProcessingService that isn't linked to the project, so "available" overclaimed there. Both now say "shown", and ProcessingService's override adds the one line callers actually need. This does need a migration (help_text is part of field state) — 0097 and 0030, schema-only. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01TngW6AshkUEhNp9D6z3U9Z --- ami/base/models.py | 21 ++++++++++++++-- .../0097_alter_taxalist_is_public.py | 24 +++++++++++++++++++ .../0030_alter_processingservice_is_public.py | 24 +++++++++++++++++++ ami/ml/models/processing_service.py | 13 +++++++--- 4 files changed, 77 insertions(+), 5 deletions(-) create mode 100644 ami/main/migrations/0097_alter_taxalist_is_public.py create mode 100644 ami/ml/migrations/0030_alter_processingservice_is_public.py diff --git a/ami/base/models.py b/ami/base/models.py index 41e7bf181..b68e3ad49 100644 --- a/ami/base/models.py +++ b/ami/base/models.py @@ -40,6 +40,23 @@ def has_many_to_many_project_relation(model: type[models.Model]) -> bool: return False +class PublicScopedModel(models.Model): + """ + Base for M2M-to-project models that can be marked public. for_project() and + visible_for_user() check issubclass(model, PublicScopedModel) for their + public-row bypass, instead of duck-typing an `is_public` attribute that any + model could grow by coincidence and silently pick up that behavior. + """ + + is_public = models.BooleanField( + default=False, + help_text="Public rows are shown to every project, not just the ones linked via 'projects'.", + ) + + class Meta: + abstract = True + + class BaseQuerySet(QuerySet): def visible_for_user(self, user: User | AnonymousUser) -> QuerySet: """ @@ -87,7 +104,7 @@ def visible_for_user(self, user: User | AnonymousUser) -> QuerySet: filter_condition |= Q(**{f"{project_field}owner": user}) | Q(**{f"{project_field}members": user}) # Public rows (e.g. public TaxaLists) are visible to everyone, draft or not. - if hasattr(model, "is_public"): + if issubclass(model, PublicScopedModel): filter_condition |= Q(is_public=True) return self.filter(filter_condition).distinct() @@ -112,7 +129,7 @@ def for_project(self, project: models.Model, include_public: bool = True) -> Que raise TypeError(f"{model.__name__} has no ManyToMany 'projects' field; for_project() is not applicable.") condition = Q(Exists(model._default_manager.filter(pk=OuterRef("pk"), projects=project))) - if include_public and hasattr(model, "is_public"): + if include_public and issubclass(model, PublicScopedModel): condition |= Q(is_public=True) return self.filter(condition) diff --git a/ami/main/migrations/0097_alter_taxalist_is_public.py b/ami/main/migrations/0097_alter_taxalist_is_public.py new file mode 100644 index 000000000..4171a32ae --- /dev/null +++ b/ami/main/migrations/0097_alter_taxalist_is_public.py @@ -0,0 +1,24 @@ +""" +Rewords TaxaList.is_public's help text from "available" to "shown": is_public +only affects visibility, not whether other subsystems still filter by project +link. Schema-only, no data change. +""" + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("main", "0096_taxalist_is_public"), + ] + + operations = [ + migrations.AlterField( + model_name="taxalist", + name="is_public", + field=models.BooleanField( + default=False, + help_text="Public rows are shown to every project, not just the ones linked via 'projects'.", + ), + ), + ] diff --git a/ami/ml/migrations/0030_alter_processingservice_is_public.py b/ami/ml/migrations/0030_alter_processingservice_is_public.py new file mode 100644 index 000000000..83535277d --- /dev/null +++ b/ami/ml/migrations/0030_alter_processingservice_is_public.py @@ -0,0 +1,24 @@ +""" +Rewords ProcessingService.is_public's help text: "available to every project" +overstated it — job dispatch, the async heartbeat, and the pipeline views still +filter by project link regardless of is_public. Schema-only, no data change. +""" + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("ml", "0029_processingservice_is_public"), + ] + + operations = [ + migrations.AlterField( + model_name="processingservice", + name="is_public", + field=models.BooleanField( + default=False, + help_text="Public services are shown to every project, not just the ones linked via 'projects'; running a job still requires the service to be linked to the project.", + ), + ), + ] diff --git a/ami/ml/models/processing_service.py b/ami/ml/models/processing_service.py index c781f9f1d..b463459e8 100644 --- a/ami/ml/models/processing_service.py +++ b/ami/ml/models/processing_service.py @@ -8,7 +8,7 @@ from django.conf import settings from django.db import models -from ami.base.models import BaseQuerySet +from ami.base.models import BaseQuerySet, PublicScopedModel from ami.main.models import BaseModel, Project from ami.ml.models.pipeline import Pipeline, get_or_create_algorithm_and_category_map from ami.ml.models.project_pipeline_config import ProjectPipelineConfig @@ -59,7 +59,7 @@ def create(self, **kwargs) -> "ProcessingService": @typing.final -class ProcessingService(BaseModel): +class ProcessingService(BaseModel, PublicScopedModel): """An ML processing service""" name = models.CharField(max_length=255) @@ -70,9 +70,16 @@ class ProcessingService(BaseModel): last_seen = models.DateTimeField(null=True) last_seen_live = models.BooleanField(null=True) last_seen_latency = models.FloatField(null=True) + # Overrides PublicScopedModel.is_public with wording specific to this model: + # unlike TaxaList, job dispatch and the async heartbeat still filter processing + # services by project link regardless of is_public, so "shown" is the accurate + # claim here, not "available". is_public = models.BooleanField( default=False, - help_text="Public processing services are available to every project, not just the ones in 'projects'.", + help_text=( + "Public services are shown to every project, not just the ones linked via 'projects'; " + "running a job still requires the service to be linked to the project." + ), ) objects = ProcessingServiceManager() From d92db94a83f48980219c97c8fd150d22a8b5f20e Mon Sep 17 00:00:00 2001 From: Michael Bunsen Date: Fri, 18 Sep 2026 07:08:34 -0700 Subject: [PATCH 12/15] test(taxa+ml): cover include_public scoping, draft-id leak, and prune the matrix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit New coverage: ?include_public=false on a detail/delete URL must not 404 a public row (TaxaList and ProcessingService); a public row linked to both a draft and a non-draft project reports only the visible project's id to an outsider, both ids to a draft-project member; get_or_create_for_project raises when given project and is_public=True together; a project member's own list can be PATCHed successfully while user_permissions reports nothing for it, pinning the known gap between the membership write gate and the guardian-based permission report (#1120) so a future change is deliberate, not accidental. test_public_list_linked_to_project_is_not_duplicated now links to three projects, matching the mechanism it guards: a naive join-based filter (Q(projects=project) | Q(is_public=True)) produces one joined row per linked project, and is_public=True is true on every one of those rows regardless of which project_id it carries, so all three pass the WHERE clause without .distinct() — verified by temporarily swapping the Exists for that Q filter and confirming the test fails with 3 rows instead of 1, then reverting. test_public_list_linked_to_multiple_projects_appears_once now runs as a superuser, since visible_for_user() returns the queryset unchanged for one (no .distinct() applied there) — a non-superuser's request benefits from that .distinct() regardless of what for_project() does, which would let a duplication bug in for_project() pass silently. Dropped ProcessingService's copy of the same guarantee; it's a property of the shared for_project() code, already covered once at the queryset level and once at the API level. Trimmed redundant permission-matrix coverage: kept one of two overlapping authenticated/anonymous "cannot update" checks per model, one of three "cannot update a scoped service" checks (all fail for the same reason — not staff), the register_pipelines allow case over its already-covered deny twin, and dropped a status test whose only difference from its sibling was an unused project_id. Split each "is_public field is read-only and reported" test into two so a failure names which behavior broke. Re-measured both list-endpoint query-count baselines after the permission and get_projects fixes above: TaxaList's 34 -> 35 (get_projects() adds one Project.objects.visible_for_user() query per request, not per row); ProcessingService's 34 -> 21 (add_processingservice_permissions() drops the per-non-public-row membership check and guardian lookup that could never grant anything to begin with). Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01TngW6AshkUEhNp9D6z3U9Z --- ami/main/tests.py | 127 ++++++++++++++++++++++++++++++++++++++++++---- ami/ml/tests.py | 120 +++++++++++++++++++++++++++++-------------- 2 files changed, 199 insertions(+), 48 deletions(-) diff --git a/ami/main/tests.py b/ami/main/tests.py index b70077c2b..531b31e52 100644 --- a/ami/main/tests.py +++ b/ami/main/tests.py @@ -5446,6 +5446,16 @@ def test_public_and_non_public_no_project_lists_with_same_name_do_not_collide(se self.assertTrue(public_list.is_public) self.assertFalse(hidden_list.is_public) + def test_project_and_is_public_together_raises(self): + """ + project and is_public=True are contradictory: a list scoped to one project + cannot also be the platform's public list of that name. Silently ignoring + is_public here (as the create path already does) would hide the caller's + mistake instead of surfacing it. + """ + with self.assertRaises(ValueError): + TaxaList.objects.get_or_create_for_project(name="Moths", project=self.project_a, is_public=True) + class TaxaListForProjectQuerySetTestCase(TestCase): """Direct unit tests for BaseQuerySet.for_project(), independent of the API layer.""" @@ -5471,7 +5481,17 @@ def test_excludes_lists_scoped_to_other_projects(self): self.assertNotIn(self.scoped.pk, ids) def test_public_list_linked_to_project_is_not_duplicated(self): - self.public.projects.add(self.project_a) + """ + A public list linked to three projects appears once when queried for one of + them. A join-based filter (Q(projects=project) | Q(is_public=True)) would + produce one joined row per linked project, and is_public=True is true on + every one of those rows regardless of which project_id it carries, so all + three would pass the WHERE clause without .distinct() — three projects is + the minimum that demonstrates this, since is_public alone can't distinguish + the row actually matching `project` from the other two. + """ + project_c = Project.objects.create(name="FP Project C") + self.public.projects.add(self.project_a, self.project_b, project_c) rows = list(TaxaList.objects.for_project(self.project_a).filter(pk=self.public.pk)) self.assertEqual(len(rows), 1) @@ -5562,10 +5582,6 @@ def test_non_member_cannot_update_public_list(self): response = self.client.patch(self._detail_url(self.public_list), {"name": "Hacked"}) self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) - def test_anonymous_cannot_update_public_list(self): - response = self.client.patch(self._detail_url(self.public_list), {"name": "Hacked"}) - self.assertIn(response.status_code, [status.HTTP_401_UNAUTHORIZED, status.HTTP_403_FORBIDDEN]) - def test_public_list_manager_can_update_public_list(self): self.client.force_authenticate(self.public_manager) response = self.client.patch(self._detail_url(self.public_list), {"name": "Renamed"}) @@ -5581,6 +5597,23 @@ def test_member_can_still_update_scoped_list(self): response = self.client.patch(self._detail_url(self.scoped_list), {"name": "Renamed"}) self.assertEqual(response.status_code, status.HTTP_200_OK) + def test_member_can_update_own_list_while_user_permissions_reports_none(self): + """ + Known gap between the write gate and the reported permissions (#1120): the + member write gate is project membership, checked directly by + IsProjectMemberOrPublicListManagerOrReadOnly. The user_permissions field + instead comes from add_m2m_object_permissions's guardian lookup, and no + per-project update_taxalist/delete_taxalist guardian grant exists for plain + membership — so a member who can successfully PATCH this list is also told, + in the same response, that they have no update permission on it. This pins + that mismatch as current, known behavior rather than a change to notice by + surprise. + """ + self.client.force_authenticate(self.member) + response = self.client.patch(self._detail_url(self.scoped_list), {"name": "Renamed Again"}) + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertNotIn("update", response.json()["user_permissions"]) + def test_public_list_manager_without_membership_cannot_update_scoped_list(self): """Holding manage_public_taxalist does not grant control over project-scoped lists.""" self.client.force_authenticate(self.public_manager) @@ -5599,6 +5632,17 @@ def test_public_list_manager_can_delete_public_list(self): response = self.client.delete(self._detail_url(self.public_list)) self.assertEqual(response.status_code, status.HTTP_204_NO_CONTENT) + def test_public_list_manager_can_delete_public_list_with_include_public_false(self): + """ + include_public=false must not make get_queryset() 404 the very row being + deleted: it governs the list action's default scope, not whether a + public row can be looked up for a detail action. + """ + self.client.force_authenticate(self.public_manager) + url = f"{self._detail_url(self.public_list)}&include_public=false" + response = self.client.delete(url) + self.assertEqual(response.status_code, status.HTTP_204_NO_CONTENT) + def test_member_can_still_delete_scoped_list(self): self.client.force_authenticate(self.member) response = self.client.delete(self._detail_url(self.scoped_list)) @@ -5623,12 +5667,14 @@ def test_non_member_can_see_public_list(self): ids = {row["id"] for row in response.json()["results"]} self.assertIn(self.public_list.pk, ids) - def test_is_public_field_is_read_only_and_reported(self): + def test_is_public_is_reported_in_the_response(self): self.client.force_authenticate(self.member) response = self.client.get(self._detail_url(self.public_list)) self.assertTrue(response.json()["is_public"]) - response = self.client.patch(self._detail_url(self.scoped_list), {"is_public": True}) + def test_is_public_cannot_be_set_through_the_api(self): + self.client.force_authenticate(self.member) + self.client.patch(self._detail_url(self.scoped_list), {"is_public": True}) self.scoped_list.refresh_from_db() self.assertFalse(self.scoped_list.is_public) @@ -5721,8 +5767,16 @@ def test_include_public_invalid_value_returns_400(self): self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) def test_public_list_linked_to_multiple_projects_appears_once(self): - """A public list attached to several projects must not be duplicated by the M2M join.""" + """ + A public list attached to several projects must not be duplicated by the M2M + join. Runs as a superuser: visible_for_user() returns the queryset unchanged + for a superuser (no .distinct() applied there), so this exercises for_project() + on its own instead of relying on visible_for_user()'s .distinct() to mask a + duplication bug in for_project() itself. + """ self.public_list.projects.add(self.project) + superuser = User.objects.create_superuser(email="include-public-super@example.com", password="testpass") + self.client.force_authenticate(superuser) rows = self._list_ids() matches = [row for row in rows if row["id"] == self.public_list.pk] self.assertEqual(len(matches), 1) @@ -5731,6 +5785,51 @@ def test_hidden_zero_project_list_is_invisible(self): ids = {row["id"] for row in self._list_ids()} self.assertNotIn(self.hidden_list.pk, ids) + def test_include_public_false_does_not_hide_a_public_list_from_retrieve(self): + """ + include_public governs the list action's default scope, not whether a + specific public row exists. ?include_public=false on a detail URL must not + 404 a public list the caller is otherwise allowed to see. + """ + response = self.client.get( + f"/api/v2/taxa/lists/{self.public_list.pk}/?project_id={self.project.pk}&include_public=false" + ) + self.assertEqual(response.status_code, status.HTTP_200_OK) + + +class TaxaListProjectsFieldVisibilityTestCase(TestCase): + """ + A public list bypasses the draft-project visibility filter that would otherwise + hide it, so its own `projects` field must not become a side channel for + disclosing a draft project's id to someone who can't see that project. + """ + + def setUp(self): + self.owner = User.objects.create_user(email="projfield-owner@example.com", password="testpass") + self.member = User.objects.create_user(email="projfield-member@example.com", password="testpass") + self.draft_project = Project.objects.create(name="Projfield Draft Project", owner=self.owner, draft=True) + self.draft_project.members.add(self.member) + self.public_project = Project.objects.create(name="Projfield Public Project", create_defaults=False) + + self.public_list = TaxaList.objects.create(name="Cross-Project Public List", is_public=True) + self.public_list.projects.add(self.draft_project, self.public_project) + + self.client = APIClient() + + def _detail_url(self): + return f"/api/v2/taxa/lists/{self.public_list.pk}/?project_id={self.public_project.pk}" + + def test_anonymous_sees_only_the_non_draft_project_id(self): + response = self.client.get(self._detail_url()) + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(response.json()["projects"], [self.public_project.pk]) + + def test_draft_project_member_sees_both_project_ids(self): + self.client.force_authenticate(self.member) + response = self.client.get(self._detail_url()) + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(set(response.json()["projects"]), {self.draft_project.pk, self.public_project.pk}) + class TaxaListDraftProjectVisibilityTestCase(TestCase): """A non-public list in a draft project follows the same visibility rule as any @@ -5808,7 +5907,12 @@ def test_member_can_add_taxon_to_list_in_draft_project(self): @override_settings(CACHALOT_ENABLED=False) class TaxaListQueryCountTestCase(APITestCase): - """Audit TaxaListViewSet.list for N+1 across a mixed public/scoped, multi-row fixture.""" + """ + Pins the current query count for TaxaListViewSet.list on a mixed public/scoped, + multi-row fixture, so a regression that adds queries is noticed. This does not + certify the absence of per-row queries — see the N+1 sources documented in the + handoff notes; it only catches a further increase from where things stand today. + """ def setUp(self): self.user = User.objects.create_user(email="qc-user@example.com", password="testpass") @@ -5829,7 +5933,10 @@ def test_list_query_count(self): response = self.client.get(url) self.assertEqual(response.status_code, status.HTTP_200_OK) self.assertEqual(len(response.json()["results"]), 5) - self.assertEqual(len(ctx.captured_queries), 34) + # 34 (previous baseline) + 1: get_projects() now resolves Project.objects + # .visible_for_user(request.user) once per request (cached on the + # serializer instance) to filter draft-project ids out of the response. + self.assertEqual(len(ctx.captured_queries), 35) class TaxaListDedupeMigrationTestCase(TestCase): diff --git a/ami/ml/tests.py b/ami/ml/tests.py index b0251e6fc..585883824 100644 --- a/ami/ml/tests.py +++ b/ami/ml/tests.py @@ -157,19 +157,6 @@ def test_check_status_without_project_id(self): self.assertEqual(response.status_code, 200) - def test_check_status_with_project_id(self): - """The other call shape: status also works when project_id is supplied.""" - service = ProcessingService.objects.create(name="Status Check Service With Project", endpoint_url=None) - service.projects.add(self.project) - url = reverse_with_params( - "api:processingservice-status", args=[service.pk], params={"project_id": self.project.pk} - ) - - self.client.force_authenticate(user=self.user) - response = self.client.get(url) - - self.assertEqual(response.status_code, 200) - def test_create_processing_service_without_endpoint_url(self): """Test creating a ProcessingService without endpoint_url (pull mode)""" processing_services_create_url = reverse_with_params( @@ -341,15 +328,6 @@ def test_member_cannot_update_scoped_service(self): response = self.client.patch(self._detail_url(self.scoped_service), {"name": "Hacked"}) self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) - def test_non_member_cannot_update_scoped_service(self): - self.client.force_authenticate(self.non_member) - response = self.client.patch(self._detail_url(self.scoped_service), {"name": "Hacked"}) - self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) - - def test_anonymous_cannot_update_scoped_service(self): - response = self.client.patch(self._detail_url(self.scoped_service), {"name": "Hacked"}) - self.assertIn(response.status_code, [status.HTTP_401_UNAUTHORIZED, status.HTTP_403_FORBIDDEN]) - def test_superuser_can_update_public_service(self): self.client.force_authenticate(self.superuser) response = self.client.patch(self._detail_url(self.public_service), {"name": "Renamed by super"}) @@ -383,6 +361,17 @@ def test_public_manager_can_delete_public_service(self): response = self.client.delete(self._detail_url(self.public_service)) self.assertEqual(response.status_code, status.HTTP_204_NO_CONTENT) + def test_public_manager_can_delete_public_service_with_include_public_false(self): + """ + include_public=false must not make get_queryset() 404 the very row being + deleted: it governs the list action's default scope, not whether a public + row can be looked up for a detail action. + """ + self.client.force_authenticate(self.public_manager) + url = f"{self._detail_url(self.public_service)}&include_public=false" + response = self.client.delete(url) + self.assertEqual(response.status_code, status.HTTP_204_NO_CONTENT) + # -- Retrieve / list visibility -- def test_anonymous_can_retrieve_public_service(self): @@ -396,12 +385,14 @@ def test_non_member_can_see_public_service(self): ids = {row["id"] for row in response.json()["results"]} self.assertIn(self.public_service.pk, ids) - def test_is_public_field_is_read_only_and_reported(self): + def test_is_public_is_reported_in_the_response(self): self.client.force_authenticate(self.staff) response = self.client.get(self._detail_url(self.public_service)) self.assertTrue(response.json()["is_public"]) - response = self.client.patch(self._detail_url(self.scoped_service), {"is_public": True}) + def test_is_public_cannot_be_set_through_the_api(self): + self.client.force_authenticate(self.staff) + self.client.patch(self._detail_url(self.scoped_service), {"is_public": True}) self.scoped_service.refresh_from_db() self.assertFalse(self.scoped_service.is_public) @@ -449,17 +440,22 @@ def test_member_cannot_register_pipelines_on_scoped_service(self): response = self.client.post(self._register_url(self.scoped_service)) self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) - def test_staff_cannot_register_pipelines_on_public_service_without_project_id(self): - """The platform-permission gate applies the same whether or not project_id is supplied.""" - self.client.force_authenticate(self.staff) - response = self.client.post(self._register_url_no_project(self.public_service)) - self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) - def test_public_manager_can_register_pipelines_on_public_service_without_project_id(self): + """The platform-permission bypass works the same whether or not project_id is supplied.""" self.client.force_authenticate(self.public_manager) response = self.client.post(self._register_url_no_project(self.public_service)) self.assertEqual(response.status_code, status.HTTP_200_OK) + def test_include_public_false_does_not_hide_a_public_service_from_retrieve(self): + """ + include_public governs the list action's default scope, not whether a + specific public row exists. ?include_public=false on a detail URL must not + 404 a public service the caller is otherwise allowed to see. + """ + url = f"{self._detail_url(self.public_service)}&include_public=false" + response = self.client.get(url) + self.assertEqual(response.status_code, status.HTTP_200_OK) + class ProcessingServiceIncludePublicParamTestCase(TestCase): """?include_public toggles whether public services appear alongside a project's own.""" @@ -502,21 +498,63 @@ def test_include_public_invalid_value_returns_400(self): ) self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) - def test_public_service_linked_to_multiple_projects_appears_once(self): - """A public service attached to several projects must not be duplicated by the M2M join.""" - self.public_service.projects.add(self.project) - rows = self._list_ids() - matches = [row for row in rows if row["id"] == self.public_service.pk] - self.assertEqual(len(matches), 1) + # The no-duplication guarantee for a public row linked to several projects is a + # property of the shared for_project()/visible_for_user() code, tested once at + # the querySet level (TaxaListForProjectQuerySetTestCase) and once at the API + # level, as a superuser, in TaxaListIncludePublicParamTestCase — no need to + # repeat it here for ProcessingService. def test_hidden_zero_project_service_is_invisible_to_non_superuser(self): ids = {row["id"] for row in self._list_ids()} self.assertNotIn(self.hidden_service.pk, ids) +class ProcessingServiceProjectsFieldVisibilityTestCase(TestCase): + """ + A public service bypasses the draft-project visibility filter that would + otherwise hide it, so its own `projects` field must not become a side channel + for disclosing a draft project's id to someone who can't see that project. + """ + + def setUp(self): + self.owner = User.objects.create_user(email="ps-projfield-owner@example.com", password="testpass") + self.member = User.objects.create_user(email="ps-projfield-member@example.com", password="testpass") + self.draft_project = Project.objects.create( + name="PS Projfield Draft Project", owner=self.owner, draft=True, create_defaults=False + ) + self.draft_project.members.add(self.member) + self.public_project = Project.objects.create(name="PS Projfield Public Project", create_defaults=False) + + self.public_service = ProcessingService.objects.create( + name="PS Cross-Project Public Service", endpoint_url=None, is_public=True + ) + self.public_service.projects.add(self.draft_project, self.public_project) + + self.client = APIClient() + + def _detail_url(self): + return f"/api/v2/ml/processing_services/{self.public_service.pk}/?project_id={self.public_project.pk}" + + def test_anonymous_sees_only_the_non_draft_project_id(self): + response = self.client.get(self._detail_url()) + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(response.json()["projects"], [self.public_project.pk]) + + def test_draft_project_member_sees_both_project_ids(self): + self.client.force_authenticate(self.member) + response = self.client.get(self._detail_url()) + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(set(response.json()["projects"]), {self.draft_project.pk, self.public_project.pk}) + + @override_settings(CACHALOT_ENABLED=False) class ProcessingServiceQueryCountTestCase(APITestCase): - """Audit ProcessingServiceViewSet.list for N+1 across a mixed public/scoped, multi-row fixture.""" + """ + Pins the current query count for ProcessingServiceViewSet.list on a mixed + public/scoped, multi-row fixture, so a regression that adds queries is + noticed. This does not certify the absence of per-row queries — it only + catches a further increase from where things stand today. + """ def setUp(self): self.user = User.objects.create_user(email="ps-qc-user@example.com", password="testpass") @@ -538,7 +576,13 @@ def test_list_query_count(self): response = self.client.get(url) self.assertEqual(response.status_code, status.HTTP_200_OK) self.assertEqual(len(response.json()["results"]), 5) - self.assertEqual(len(ctx.captured_queries), 34) + # 34 (previous baseline) down to 21: add_processingservice_permissions() no + # longer runs the M2M membership check or the guardian get_perms() lookup + # per non-public row (no per-project *_processingservice guardian permission + # exists, so that branch could only ever fire for a superuser, which a plain + # attribute check covers for free); get_projects() adds back one query per + # request (not per row) for the draft-project-id visibility filter. + self.assertEqual(len(ctx.captured_queries), 21) class TestProjectPipelineRegistrationUpdatesLastSeen(APITestCase): From 9158039c982ede47588140e4989a03889a97fd6b Mon Sep 17 00:00:00 2001 From: Michael Bunsen Date: Fri, 18 Sep 2026 07:35:09 -0700 Subject: [PATCH 13/15] refactor(base): consolidate the public-row write-permission helpers into one check_taxalist_write_permission and check_processingservice_write_permission differed only in the fallback used for a non-public row (project membership vs. active-staff status); IsProjectMemberOrPublicListManager[OrReadOnly] and IsActiveStaffOrPublicManager[OrReadOnly] were the same shape again, parameterised the same way. Both pairs are now one shared function/base class taking that fallback as a predicate, with every existing name kept as a thin subclass or alias so the branches stacked on this one still import cleanly. Nothing was renamed. Also fixes the one remaining bare `# type: ignore` on the superuser check this consolidation touched, matching its siblings' `[union-attr]` form. The four permission classes' full test matrices pass unchanged. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01TngW6AshkUEhNp9D6z3U9Z --- ami/base/permissions.py | 151 +++++++++++++++++++--------------------- 1 file changed, 71 insertions(+), 80 deletions(-) diff --git a/ami/base/permissions.py b/ami/base/permissions.py index 0f0dfc764..456844047 100644 --- a/ami/base/permissions.py +++ b/ami/base/permissions.py @@ -91,30 +91,30 @@ def user_can_manage_public(user: AbstractBaseUser | AnonymousUser, model_or_inst return user.has_perm(f"{meta.app_label}.manage_public_{meta.model_name}") # type: ignore[union-attr] -def check_taxalist_write_permission(user, taxa_list, project) -> bool: +def check_public_scoped_write_permission(user, instance, non_public_fallback) -> bool: """ - True if `user` may update/delete `taxa_list` or change its taxa: a public - list needs manage_public_taxalist, a project-scoped one needs membership of - `project`. Kept TaxaList-specific rather than folded into a generic helper — - ProcessingService's non-public fallback is active-staff status, not project - membership, so the two checks don't share a body. + True if `user` may write to `instance`: a public row needs the platform + manage_public_ permission (superusers always pass, via + user_can_manage_public); a non-public row falls back to `non_public_fallback()`, + the model's own write rule (project membership, active-staff status, ...). """ - if getattr(taxa_list, "is_public", False): - return user_can_manage_public(user, taxa_list) - return bool(user.is_superuser or (project and project.members.filter(pk=user.pk).exists())) + if getattr(instance, "is_public", False): + return user_can_manage_public(user, instance) + return non_public_fallback() + + +def check_taxalist_write_permission(user, taxa_list, project) -> bool: + """Thin alias kept so branches stacked on this one still import this name.""" + return check_public_scoped_write_permission( + user, taxa_list, lambda: bool(user.is_superuser or (project and project.members.filter(pk=user.pk).exists())) + ) def check_processingservice_write_permission(user, processing_service) -> bool: - """ - True if `user` may update/delete `processing_service`, or register its - pipelines: a public service needs manage_public_processingservice; a - project-scoped one only needs active-staff status, unchanged from before - is_public existed (the broader project-scoped permission model belongs to - #1120, not here). - """ - if getattr(processing_service, "is_public", False): - return user_can_manage_public(user, processing_service) - return bool(user.is_superuser or is_active_staff(user)) + """Thin alias kept so branches stacked on this one still import this name.""" + return check_public_scoped_write_permission( + user, processing_service, lambda: bool(user.is_superuser or is_active_staff(user)) + ) def add_processingservice_permissions(user, instance, response_data: dict) -> dict: @@ -213,14 +213,26 @@ def has_permission(self, request, view): return project.members.filter(pk=request.user.pk).exists() -class IsProjectMemberOrPublicListManager(permissions.BasePermission): +class _BaseGateOrPublicManager(permissions.BasePermission): """ - Safe methods are open to everyone. Unsafe methods need membership of the - active project, or the manage_public_taxalist permission — used by the - nested add/remove-taxon route, which serves both public and project-scoped - lists and has no object to check yet at has_permission() time. + Shared shape for M2M-to-project models with a public flag: safe methods are + open to everyone; unsafe methods need the model's own base gate (project + membership, active-staff status, ...) or the manage_public_ platform + permission. `exclude_create_from_bypass` forces a plain "create a new row" + action through the base gate only, since there's no object yet to tell + whether it will be public. Subclasses implement get_model() and + get_base_gate() — get_model() does a local import to avoid a module-level + circular import between this file and the app that owns the model. """ + exclude_create_from_bypass = False + + def get_model(self): + raise NotImplementedError + + def get_base_gate(self, request, view) -> bool: + raise NotImplementedError + def has_permission(self, request, view): if request.method in permissions.SAFE_METHODS: return True @@ -228,84 +240,63 @@ def has_permission(self, request, view): if not request.user or not request.user.is_authenticated: return False - from ami.main.models import TaxaList - - if request.user.is_superuser or user_can_manage_public(request.user, TaxaList): # type: ignore[union-attr] + if request.user.is_superuser: # type: ignore[union-attr] return True - get_active_project = getattr(view, "get_active_project", None) - if not get_active_project: - return False + if self.exclude_create_from_bypass and getattr(view, "action", None) == "create": + return self.get_base_gate(request, view) - project = get_active_project() - if not project: - return False - - return project.members.filter(pk=request.user.pk).exists() - - -class IsProjectMemberOrPublicListManagerOrReadOnly(IsProjectMemberOrPublicListManager): - """ - For TaxaListViewSet: creating a brand-new list always needs real project - membership (there's no object yet to tell whether it will be public). - Update/delete defer to check_taxalist_write_permission, which grants the - manage_public_taxalist bypass only once the target list's is_public flag - is known. - """ + if user_can_manage_public(request.user, self.get_model()): + return True - def has_permission(self, request, view): - if getattr(view, "action", None) == "create": - return IsProjectMemberOrReadOnly.has_permission(self, request, view) - return super().has_permission(request, view) + return self.get_base_gate(request, view) def has_object_permission(self, request, view, obj): if request.method in permissions.SAFE_METHODS: return True - project = view.get_active_project() if hasattr(view, "get_active_project") else None - return check_taxalist_write_permission(request.user, obj, project) + return check_public_scoped_write_permission(request.user, obj, lambda: self.get_base_gate(request, view)) -class IsActiveStaffOrPublicManager(permissions.BasePermission): +class IsProjectMemberOrPublicListManager(_BaseGateOrPublicManager): """ - Safe methods are open to everyone. Unsafe methods need active-staff status, - or the manage_public_processingservice permission — used where there is no - object yet to check at has_permission() time. + Used by the nested add/remove-taxon route: serves both public and + project-scoped lists, and has no object to check yet at has_permission() + time. """ - def has_permission(self, request, view): - if request.method in permissions.SAFE_METHODS: - return True + def get_model(self): + from ami.main.models import TaxaList - if not request.user or not request.user.is_authenticated: - return False + return TaxaList + def get_base_gate(self, request, view): + get_active_project = getattr(view, "get_active_project", None) + project = get_active_project() if get_active_project else None + return bool(project and project.members.filter(pk=request.user.pk).exists()) + + +class IsProjectMemberOrPublicListManagerOrReadOnly(IsProjectMemberOrPublicListManager): + """For TaxaListViewSet: creating a brand-new list always needs real project membership.""" + + exclude_create_from_bypass = True + + +class IsActiveStaffOrPublicManager(_BaseGateOrPublicManager): + """Used by ProcessingServiceViewSet's non-create actions and any future nested route.""" + + def get_model(self): from ami.ml.models.processing_service import ProcessingService - if request.user.is_superuser or user_can_manage_public(request.user, ProcessingService): # type: ignore - return True + return ProcessingService - return is_active_staff(request.user) # type: ignore[arg-type] + def get_base_gate(self, request, view): + return is_active_staff(request.user) class IsActiveStaffOrPublicManagerOrReadOnly(IsActiveStaffOrPublicManager): - """ - For ProcessingServiceViewSet: creating a brand-new service always needs - active-staff status (there's no object yet to tell whether it will be - public). Update/delete/register_pipelines defer to - check_processingservice_write_permission, which grants the - manage_public_processingservice bypass only once the target's is_public - flag is known. - """ + """For ProcessingServiceViewSet: creating a brand-new service always needs active-staff status.""" - def has_permission(self, request, view): - if getattr(view, "action", None) == "create": - return IsActiveStaffOrReadOnly.has_permission(self, request, view) - return super().has_permission(request, view) - - def has_object_permission(self, request, view, obj): - if request.method in permissions.SAFE_METHODS: - return True - return check_processingservice_write_permission(request.user, obj) + exclude_create_from_bypass = True class ObjectPermission(permissions.BasePermission): From 38874ef7cab4b5844a28fdf3425e52894f47d069 Mon Sep 17 00:00:00 2001 From: Michael Bunsen Date: Fri, 18 Sep 2026 07:59:12 -0700 Subject: [PATCH 14/15] chore(migrations): fold the help-text change into the migrations that add the field The branch has not been deployed anywhere, so a separate migration whose only effect is new help text adds a file for nothing and takes migration numbers that stacked branches already use. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01TngW6AshkUEhNp9D6z3U9Z --- .../migrations/0096_taxalist_is_public.py | 2 +- .../0097_alter_taxalist_is_public.py | 24 ------------------- .../0029_processingservice_is_public.py | 2 +- .../0030_alter_processingservice_is_public.py | 24 ------------------- 4 files changed, 2 insertions(+), 50 deletions(-) delete mode 100644 ami/main/migrations/0097_alter_taxalist_is_public.py delete mode 100644 ami/ml/migrations/0030_alter_processingservice_is_public.py diff --git a/ami/main/migrations/0096_taxalist_is_public.py b/ami/main/migrations/0096_taxalist_is_public.py index 650f40f23..397feece1 100644 --- a/ami/main/migrations/0096_taxalist_is_public.py +++ b/ami/main/migrations/0096_taxalist_is_public.py @@ -42,7 +42,7 @@ class Migration(migrations.Migration): name="is_public", field=models.BooleanField( default=False, - help_text="Public lists are available to every project, not just the ones in 'projects'.", + help_text="Public rows are shown to every project, not just the ones linked via 'projects'.", ), ), migrations.AlterField( diff --git a/ami/main/migrations/0097_alter_taxalist_is_public.py b/ami/main/migrations/0097_alter_taxalist_is_public.py deleted file mode 100644 index 4171a32ae..000000000 --- a/ami/main/migrations/0097_alter_taxalist_is_public.py +++ /dev/null @@ -1,24 +0,0 @@ -""" -Rewords TaxaList.is_public's help text from "available" to "shown": is_public -only affects visibility, not whether other subsystems still filter by project -link. Schema-only, no data change. -""" - -from django.db import migrations, models - - -class Migration(migrations.Migration): - dependencies = [ - ("main", "0096_taxalist_is_public"), - ] - - operations = [ - migrations.AlterField( - model_name="taxalist", - name="is_public", - field=models.BooleanField( - default=False, - help_text="Public rows are shown to every project, not just the ones linked via 'projects'.", - ), - ), - ] diff --git a/ami/ml/migrations/0029_processingservice_is_public.py b/ami/ml/migrations/0029_processingservice_is_public.py index 73e4626d5..fd623cccb 100644 --- a/ami/ml/migrations/0029_processingservice_is_public.py +++ b/ami/ml/migrations/0029_processingservice_is_public.py @@ -29,7 +29,7 @@ class Migration(migrations.Migration): name="is_public", field=models.BooleanField( default=False, - help_text="Public processing services are available to every project, not just the ones in 'projects'.", + help_text="Public services are shown to every project, not just the ones linked via 'projects'; running a job still requires the service to be linked to the project.", ), ), ] diff --git a/ami/ml/migrations/0030_alter_processingservice_is_public.py b/ami/ml/migrations/0030_alter_processingservice_is_public.py deleted file mode 100644 index 83535277d..000000000 --- a/ami/ml/migrations/0030_alter_processingservice_is_public.py +++ /dev/null @@ -1,24 +0,0 @@ -""" -Rewords ProcessingService.is_public's help text: "available to every project" -overstated it — job dispatch, the async heartbeat, and the pipeline views still -filter by project link regardless of is_public. Schema-only, no data change. -""" - -from django.db import migrations, models - - -class Migration(migrations.Migration): - dependencies = [ - ("ml", "0029_processingservice_is_public"), - ] - - operations = [ - migrations.AlterField( - model_name="processingservice", - name="is_public", - field=models.BooleanField( - default=False, - help_text="Public services are shown to every project, not just the ones linked via 'projects'; running a job still requires the service to be linked to the project.", - ), - ), - ] From 083913f679067402aaafa26bcf2f24b70a2fcb02 Mon Sep 17 00:00:00 2001 From: Michael Bunsen Date: Tue, 6 Oct 2026 16:04:12 -0700 Subject: [PATCH 15/15] test(taxa): repin the taxa list query count after the list speedup, and check it does not grow with rows The Taxa Lists speedup from main brings the list down from 35 queries to 13. The test now warms process-wide caches first, so the count no longer depends on which tests ran before it, and asserts that ten rows cost the same as five, which catches a per-row lookup on either a public or a project-scoped list. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01TngW6AshkUEhNp9D6z3U9Z --- ami/main/tests.py | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/ami/main/tests.py b/ami/main/tests.py index 7e9f6c0d2..b1b3c46f9 100644 --- a/ami/main/tests.py +++ b/ami/main/tests.py @@ -6037,10 +6037,9 @@ def test_member_can_add_taxon_to_list_in_draft_project(self): @override_settings(CACHALOT_ENABLED=False) class TaxaListQueryCountTestCase(APITestCase): """ - Pins the current query count for TaxaListViewSet.list on a mixed public/scoped, - multi-row fixture, so a regression that adds queries is noticed. This does not - certify the absence of per-row queries — see the N+1 sources documented in the - handoff notes; it only catches a further increase from where things stand today. + Pins the query count for TaxaListViewSet.list on a mixed public/scoped fixture, + and checks that doubling the rows adds no queries, so a per-row lookup on either + kind of list is caught. """ def setUp(self): @@ -6054,18 +6053,28 @@ def setUp(self): self.client = APIClient() self.client.force_authenticate(self.user) - def test_list_query_count(self): + def _count_list_queries(self, expected_rows: int) -> int: from cachalot.api import cachalot_disabled url = f"/api/v2/taxa/lists/?project_id={self.project.pk}" with cachalot_disabled(), CaptureQueriesContext(connection) as ctx: response = self.client.get(url) self.assertEqual(response.status_code, status.HTTP_200_OK) - self.assertEqual(len(response.json()["results"]), 5) - # 34 (previous baseline) + 1: get_projects() now resolves Project.objects - # .visible_for_user(request.user) once per request (cached on the - # serializer instance) to filter draft-project ids out of the response. - self.assertEqual(len(ctx.captured_queries), 35) + self.assertEqual(len(response.json()["results"]), expected_rows) + return len(ctx.captured_queries) + + def test_list_query_count(self): + # Warm process-wide caches (content types, permissions) so the pinned + # count does not depend on which tests ran earlier in the suite. + self._count_list_queries(expected_rows=5) + five_rows = self._count_list_queries(expected_rows=5) + for i in range(3, 6): + TaxaList.objects.create(name=f"Scoped {i}").projects.add(self.project) + for i in range(2, 4): + TaxaList.objects.create(name=f"Public {i}", is_public=True) + ten_rows = self._count_list_queries(expected_rows=10) + + self.assertEqual((five_rows, ten_rows), (13, 13)) class TaxaListDedupeMigrationTestCase(TestCase):