diff --git a/backend/api_v2/api_deployment_views.py b/backend/api_v2/api_deployment_views.py index 8f5d0763a9..46ddc67953 100644 --- a/backend/api_v2/api_deployment_views.py +++ b/backend/api_v2/api_deployment_views.py @@ -3,12 +3,14 @@ import uuid from typing import Any -from django.db.models import F, OuterRef, QuerySet, Subquery +from django.db.models import Count, F, IntegerField, OuterRef, QuerySet, Subquery +from django.db.models.functions import Coalesce from django.http import HttpResponse from permissions.membership_views import OwnerManagementMixin from permissions.permission import IsOwner, IsOwnerOrSharedUserOrSharedToOrg from permissions.resource_share_views import ResourceShareManagementMixin from permissions.roles import ResourceRole +from platform_api.openapi_schema import PlatformKeyAutoSchema from plugins import get_plugin from prompt_studio.prompt_studio_registry_v2.models import PromptStudioRegistry from rest_framework import serializers, status, views, viewsets @@ -33,7 +35,10 @@ contains_tool_not_found_error, ) from api_v2.models import APIDeployment -from api_v2.openapi_schema import DEPLOYMENT_EXECUTION_SCHEMA +from api_v2.openapi_schema import ( + API_DEPLOYMENT_LIST_SCHEMA, + DEPLOYMENT_EXECUTION_SCHEMA, +) from api_v2.rate_limiter import APIDeploymentRateLimiter from api_v2.serializers import ( APIDeploymentListSerializer, @@ -246,10 +251,17 @@ def get( ) +@API_DEPLOYMENT_LIST_SCHEMA class APIDeploymentViewSet( OwnerManagementMixin, ResourceShareManagementMixin, viewsets.ModelViewSet ): pagination_class = CustomPagination + + # For schema generation only; get_queryset replaces it on every request. + queryset = APIDeployment.objects.none() + + # Error examples have to match what the auth middleware sends. + schema = PlatformKeyAutoSchema() notification_resource_name_field = "display_name" def get_notification_resource_type(self, resource: Any) -> str | None: @@ -275,19 +287,42 @@ def get_queryset(self) -> QuerySet | None: .order_by("-created_at") .values("created_at")[:1] ) + run_count_subquery = ( + WorkflowExecution.objects.filter(pipeline_id=OuterRef("id")) + .values("pipeline_id") + .annotate(total=Count("id")) + .values("total") + ) # Avoid per-row queries for owner/co-owner + creator fields in list views queryset = ( APIDeployment.objects.for_user(self.request.user) .select_related("created_by") .prefetch_related("memberships__user") - .annotate(last_run_time_annotated=Subquery(last_run_subquery)) - .order_by(F("last_run_time_annotated").desc(nulls_last=True)) + .annotate( + last_run_time_annotated=Subquery(last_run_subquery), + run_count_annotated=Coalesce( + Subquery(run_count_subquery, output_field=IntegerField()), 0 + ), + ) + # `pk` last because the primary ordering ties on every deployment + # that has never run, and a paging client would then see a row + # twice or not at all. + .order_by(F("last_run_time_annotated").desc(nulls_last=True), "pk") ) - # Filter by workflow ID if provided + # TODO: replace the hand-read params and their OpenApiParameter + # restatements with a FilterSet so the spec cannot drift from the code workflow_filter = self.request.query_params.get("workflow", None) if workflow_filter: + try: + uuid.UUID(workflow_filter) + except ValueError: + # Django raises on evaluation, past the handler that turns a bad + # request into a 400. + raise serializers.ValidationError( + {"workflow": "Must be a valid UUID."} + ) from None queryset = queryset.filter(workflow_id=workflow_filter) # Search by display name diff --git a/backend/api_v2/deployment_spec_urls.py b/backend/api_v2/deployment_spec_urls.py index 727997ee0f..54d0d98ccc 100644 --- a/backend/api_v2/deployment_spec_urls.py +++ b/backend/api_v2/deployment_spec_urls.py @@ -9,8 +9,11 @@ ``@extend_schema`` and adding its urlconf here. """ +from django.conf import settings from django.core.exceptions import ImproperlyConfigured +from django.urls import path +from api_v2.api_deployment_views import APIDeploymentViewSet from backend import base_urls SPEC_URLCONFS = ("api_v2.execution_urls", "platform_api.whoami_urls") @@ -27,3 +30,14 @@ f"{', '.join(sorted(missing))} is not mounted in backend.base_urls; the " "spec would be generated for routes the server does not serve." ) + +# Restated rather than selected: this route is served from a urlconf carrying +# routes that are not published, and only its GET is. Both halves of the +# restatement are held to the served route by `test_docstudio_spec`. +urlpatterns += [ + path( + f"{settings.TENANT_SUBFOLDER_PREFIX}/api/deployment/", + APIDeploymentViewSet.as_view({"get": "list"}), + name="api_deployment", + ), +] diff --git a/backend/api_v2/management/commands/generate_docstudio_spec.py b/backend/api_v2/management/commands/generate_docstudio_spec.py index a4ff64f269..be2278e806 100644 --- a/backend/api_v2/management/commands/generate_docstudio_spec.py +++ b/backend/api_v2/management/commands/generate_docstudio_spec.py @@ -14,9 +14,11 @@ """ import json +import re from pathlib import Path from typing import Any +from django.conf import settings from django.core.management.base import BaseCommand, CommandError from drf_spectacular.drainage import GENERATOR_STATS from drf_spectacular.generators import SchemaGenerator @@ -35,7 +37,11 @@ # is exactly `TENANT_SUBFOLDER_PREFIX`, so with a `startswith` over the union an # `API_DEPLOYMENT_PATH_PREFIX` pointed anywhere under the tenant mount -- # `api/v1/unstract/deploy`, say -- passed the gate this comment says it fails. -PUBLISHED_PATH_PREFIXES = ("deployment", "api/v1/unstract/whoami") +PUBLISHED_PATH_PREFIXES = ( + "deployment", + "api/v1/unstract/whoami", + "api/v1/unstract/{org_id}/api/deployment", +) # Named in every failure message: the repos that regenerate from this file are # the ones a spec change actually breaks, and nothing there watches this repo. DOWNSTREAM = ( @@ -44,6 +50,43 @@ "PRs there for anything that changes an operation id, a tag or a schema." ) +# A literal for the same reason as `PUBLISHED_PATH_PREFIXES`. +TENANT_MOUNT = "/api/v1/unstract/" +ORG_SEGMENT = "{org_id}" +HTTP_METHODS = frozenset( + {"get", "put", "post", "delete", "options", "head", "patch", "trace"} +) +ORG_SEGMENT_PARAMETER = { + "in": "path", + "name": "org_id", + "required": True, + "schema": {"type": "string"}, + "description": ( + "The organisation the request is scoped to, as `whoami` reports it in " + "`organization_id`." + ), +} + + +def _restore_organisation_segment(schema: dict[str, Any]) -> None: + """Put back the organisation segment the router never sees. + + `OrganizationMiddleware` strips it before routing, so paths taken from the + URLconf are not the ones callers send. The routes genuinely served without + it are the ones that setting whitelists, so it decides this too. + """ + for url in [url for url in schema["paths"] if url.startswith(TENANT_MOUNT)]: + if any( + re.match(whitelisted, url) + for whitelisted in settings.ORGANIZATION_MIDDLEWARE_WHITELISTED_PATHS + ): + continue + item = schema["paths"].pop(url) + for method, operation in item.items(): + if method in HTTP_METHODS: + operation.setdefault("parameters", []).append(dict(ORG_SEGMENT_PARAMETER)) + schema["paths"][f"{TENANT_MOUNT}{ORG_SEGMENT}/{url[len(TENANT_MOUNT):]}"] = item + class SpecGenerationFailed(CommandError): """Raised when the generator had to guess.""" @@ -76,6 +119,8 @@ def render_spec() -> str: f"API nobody implements:\n{diagnostics}" ) + _restore_organisation_segment(schema) + published = tuple(f"/{prefix}/" for prefix in PUBLISHED_PATH_PREFIXES) off_prefix = [path for path in schema["paths"] if not path.startswith(published)] if off_prefix: diff --git a/backend/api_v2/openapi_schema.py b/backend/api_v2/openapi_schema.py index d3376276a0..34e63ce2b4 100644 --- a/backend/api_v2/openapi_schema.py +++ b/backend/api_v2/openapi_schema.py @@ -15,9 +15,12 @@ extend_schema_serializer, extend_schema_view, ) +from platform_api.openapi_schema import PlatformKeyError from rest_framework import serializers +from api_v2.models import API_NAME_MAX_LENGTH, DESCRIPTION_MAX_LENGTH from api_v2.serializers import ( + APIDeploymentListSerializer, APIExecutionResponseSerializer, ExecutionQuerySerializer, ExecutionRequestSerializer, @@ -170,8 +173,12 @@ class ErrorResponse(serializers.Serializer): ), } +EXECUTE_SUMMARY = "Execute an API deployment against documents" + EXECUTE_DESCRIPTION = ( - "Execute an API deployment against one or more documents.\n\n" + "Runs an API deployment. Takes a deployment key — either the " + "deployment's own key, or a global API deployment key that has access " + "to it.\n\n" "Supply the documents either as `files` (multipart upload) or as " "`presigned_urls` (HTTPS S3 URLs), or both — a request carrying neither is " f"rejected, and the two together may not exceed " @@ -180,15 +187,16 @@ class ErrorResponse(serializers.Serializer): "execution is queued; read the outcome from the status endpoint." ) +STATUS_SUMMARY = "Read the result of an execution" + STATUS_DESCRIPTION = ( - "Read the result of a previously started execution.\n\n" - "This read is one-shot: the first call that observes a completed execution " - "acknowledges it and the stored result is discarded, so every later call " - "for that execution answers 406. Poll while the execution is pending, and " - "keep the payload of the call that returns it — it cannot be fetched again." - "\n\nA still-running execution answers 422 carrying its current `status`, " - "so a polling loop should treat 422 as the normal reply and stop on 200. " - "Clients that raise on any non-2xx need to allow for that." + "Reads a previously started execution, taking the same deployment key " + "that ran it. The read is one-shot: the first call that observes a completed " + "execution acknowledges it and the stored result is discarded, so every " + "later call for that execution answers 406. Keep the payload of the call " + "that returns it — it cannot be fetched again.\n\n" + "A still-running execution answers 422 carrying its current `status`, so a " + "polling loop should treat 422 as the normal reply and stop on 200." ) @@ -197,6 +205,7 @@ class ErrorResponse(serializers.Serializer): DEPLOYMENT_EXECUTION_SCHEMA = extend_schema_view( post=extend_schema( operation_id="execute", + summary=EXECUTE_SUMMARY, tags=["deployment"], auth=DEPLOYMENT_AUTH, parameters=DEPLOYMENT_PATH_PARAMETERS, @@ -218,6 +227,7 @@ class ErrorResponse(serializers.Serializer): ), get=extend_schema( operation_id="status", + summary=STATUS_SUMMARY, tags=["deployment"], auth=DEPLOYMENT_AUTH, parameters=DEPLOYMENT_PATH_PARAMETERS + [ExecutionQuerySerializer], @@ -242,3 +252,98 @@ class ErrorResponse(serializers.Serializer): description=STATUS_DESCRIPTION, ), ) + + +# Declares no field of its own, so a change to the real serializer moves the +# spec. It carries a caller-facing description and a component name the +# pagination envelope can be wrapped round without stuttering. +@extend_schema_serializer(component_name="APIDeploymentSummary") +class APIDeploymentSummary(APIDeploymentListSerializer): + """One API deployment, as it appears in an organisation's listing. + + `api_name` and `api_endpoint` are what an execution call needs; + `display_name` and `description` say what the deployment is for. + """ + + # The model defaults these, so DRF reports them optional --- true of a + # request body, wrong for a response the server always fills. Left alone, + # a generated client types them nullable and every caller writes a check + # for a key that is always there. The lengths are restated because DRF drops + # them from a read-only field. + api_name = serializers.CharField(read_only=True, max_length=API_NAME_MAX_LENGTH) + display_name = serializers.CharField(read_only=True, max_length=API_NAME_MAX_LENGTH) + description = serializers.CharField(read_only=True, max_length=DESCRIPTION_MAX_LENGTH) + is_active = serializers.BooleanField(read_only=True) + + +# This operation takes a platform key rather than a deployment key, so its +# credential failures come from the auth middleware in `PlatformKeyError` shape +# rather than from the project exception handler. +API_DEPLOYMENT_LIST_QUERY_PARAMETERS = [ + OpenApiParameter( + "workflow", + {"type": "string", "format": "uuid"}, + OpenApiParameter.QUERY, + description="Return only the deployments of this workflow.", + ), + OpenApiParameter( + "api_name", + {"type": "string"}, + OpenApiParameter.QUERY, + description="Return only the deployment with exactly this `api_name`.", + ), + OpenApiParameter( + "search", + {"type": "string"}, + OpenApiParameter.QUERY, + description="Return only deployments whose display name contains this " + "text, case-insensitively.", + ), +] + +LIST_API_DEPLOYMENTS_SUMMARY = "List an organisation's API deployments" + +LIST_API_DEPLOYMENTS_DESCRIPTION = ( + "Lists what an organisation has deployed, authenticated by a platform API " + "key that belongs to it.\n\n" + "A deployment key is not part of this listing; executing a deployment " + "still needs one.\n\n" + "Ordered by most recent run first, then by identifier, so paging is stable." +) + + +API_DEPLOYMENT_LIST_SCHEMA = extend_schema_view( + list=extend_schema( + operation_id="list_deployments", + summary=LIST_API_DEPLOYMENTS_SUMMARY, + tags=["deployment"], + auth=[{"platformKey": []}], + parameters=API_DEPLOYMENT_LIST_QUERY_PARAMETERS, + responses={ + 200: OpenApiResponse( + APIDeploymentSummary(many=True), + description="One page of the organisation's API deployments.", + ), + 400: OpenApiResponse( + ErrorResponse, + description="A query parameter was malformed.", + ), + 401: OpenApiResponse( + PlatformKeyError, + description="No usable platform API key was supplied — absent, " + "malformed, unknown, or revoked.", + ), + 403: OpenApiResponse( + PlatformKeyError, + description="The key was recognised but refused: it does not " + "belong to the organisation named in the path, or its " + "permission tier is not one this deployment knows.", + ), + 500: OpenApiResponse( + description="The request could not be served. The body is not " + "guaranteed to be JSON.", + ), + }, + description=LIST_API_DEPLOYMENTS_DESCRIPTION, + ), +) diff --git a/backend/api_v2/serializers.py b/backend/api_v2/serializers.py index 7a6014c845..31c609ebaa 100644 --- a/backend/api_v2/serializers.py +++ b/backend/api_v2/serializers.py @@ -499,6 +499,9 @@ def validate_execution_id(self, value): return str(uuid_obj) +_UNANNOTATED = object() + + class APIDeploymentListSerializer(ModelSerializer): workflow_name = CharField(source="workflow.workflow_name", read_only=True) created_by_email = SerializerMethodField() @@ -528,7 +531,7 @@ class Meta: "co_owners_count", ] - def get_created_by_email(self, obj): + def get_created_by_email(self, obj) -> str | None: """Get the email of the creator.""" return obj.created_by.email if obj.created_by else None @@ -539,12 +542,20 @@ def get_is_owner(self, obj) -> bool: def get_co_owners_count(self, obj) -> int: return obj.co_owners_count() + # Both read the list view's annotations when they are there, and fall back + # to a query for the callers that serialize a plain queryset. A deployment + # that has never run annotates to `None`, so absence is what decides, not + # the value. def get_run_count(self, instance) -> int: - """Get total execution count for this API deployment.""" + annotated = getattr(instance, "run_count_annotated", _UNANNOTATED) + if annotated is not _UNANNOTATED: + return annotated return WorkflowExecution.objects.filter(pipeline_id=instance.id).count() def get_last_run_time(self, instance) -> str | None: - """Get the timestamp of the most recent execution.""" + annotated = getattr(instance, "last_run_time_annotated", _UNANNOTATED) + if annotated is not _UNANNOTATED: + return annotated.isoformat() if annotated else None last_execution = ( WorkflowExecution.objects.filter(pipeline_id=instance.id) .order_by("-created_at") diff --git a/backend/api_v2/tests/test_deployment_listing.py b/backend/api_v2/tests/test_deployment_listing.py new file mode 100644 index 0000000000..6e7c846866 --- /dev/null +++ b/backend/api_v2/tests/test_deployment_listing.py @@ -0,0 +1,269 @@ +"""Request-level tests for listing an organisation's API deployments. + +What makes a platform key work here sits either side of the view: the key +resolves to a service account, the organisation comes out of the path, and the +manager scopes the queryset to it. None of that is visible from the view alone, +so these go through the real URLconf and a real middleware chain. +""" + +import secrets +import uuid + +from account_v2.models import Organization, User +from django.conf import settings +from django.db import connection +from django.test import override_settings +from django.test.utils import CaptureQueriesContext +from platform_api.models import ApiKeyPermission, PlatformApiKey +from platform_api.services import create_api_user_for_key +from rest_framework.test import APITestCase +from utils.user_context import UserContext +from workflow_manager.workflow_v2.models.execution import WorkflowExecution +from workflow_manager.workflow_v2.models.workflow import Workflow + +from api_v2.models import APIDeployment +from api_v2.serializers import APIDeploymentListSerializer + +ORG_A = "org-a" +ORG_B = "org-b" + + +def listing_url(organization_id: str) -> str: + return f"/{settings.PATH_PREFIX}/unstract/{organization_id}/api/deployment/" + + +# Trimmed from the production chain, preserving its relative order. The cloud +# test settings drop CustomAuthMiddleware, so pinning the list keeps this suite +# behaving the same in both trees. +_MIDDLEWARE = [ + "middleware.request_id.CustomRequestIDMiddleware", + settings.TENANT_MIDDLEWARE, + "django.contrib.sessions.middleware.SessionMiddleware", + "django.middleware.csrf.CsrfViewMiddleware", + "django.contrib.auth.middleware.AuthenticationMiddleware", + settings.CUSTOM_AUTH_MIDDLEWARE, +] + + +@override_settings(MIDDLEWARE=_MIDDLEWARE) +class DeploymentListingTest(APITestCase): + def setUp(self) -> None: + self.org_a = Organization.objects.create( + name=ORG_A, display_name="Org A", organization_id=ORG_A + ) + self.org_b = Organization.objects.create( + name=ORG_B, display_name="Org B", organization_id=ORG_B + ) + self.deployment = self._make_deployment( + self.org_a, api_name="invoices", description="Reads an invoice." + ) + + def tearDown(self) -> None: + # Left set, this thread-local scopes the managers in whatever runs next. + UserContext.set_organization_identifier(None) + + def _make_deployment(self, organization, **kwargs) -> APIDeployment: + creator = self._make_user() + workflow = Workflow.objects.create( + workflow_name=f"wf-{uuid.uuid4().hex[:8]}", + organization=organization, + created_by=creator, + ) + # `save()` composes `api_endpoint` from the thread-local, not the row. + UserContext.set_organization_identifier(organization.organization_id) + try: + return APIDeployment.objects.create( + display_name=kwargs.pop("display_name", "Invoices"), + workflow=workflow, + organization=organization, + created_by=creator, + **kwargs, + ) + finally: + UserContext.set_organization_identifier(None) + + @staticmethod + def _make_user() -> User: + email = f"user-{uuid.uuid4().hex[:8]}@example.com" + return User.objects.create_user( + username=email, email=email, password=secrets.token_urlsafe() + ) + + def _make_key(self, organization=None, **kwargs) -> PlatformApiKey: + key = PlatformApiKey.objects.create( + name=f"key-{uuid.uuid4().hex[:8]}", + description="test key", + organization=organization or self.org_a, + **kwargs, + ) + # The minting path, for the `is_service_account` flag it sets. + create_api_user_for_key(key, key.organization) + return key + + def _get(self, token: str | None = None, organization_id: str = ORG_A): + headers = {"HTTP_AUTHORIZATION": f"Bearer {token}"} if token else {} + return self.client.get(listing_url(organization_id), **headers) + + def test_a_key_lists_its_organisations_deployments(self) -> None: + response = self._get(str(self._make_key().key)) + + self.assertEqual(response.status_code, 200) + body = response.json() + self.assertEqual(body["count"], 1) + (listed,) = body["results"] + self.assertEqual(listed["api_name"], "invoices") + self.assertEqual(listed["display_name"], "Invoices") + self.assertEqual(listed["description"], "Reads an invoice.") + self.assertEqual(listed["api_endpoint"], f"deployment/api/{ORG_A}/invoices/") + + def test_the_listing_carries_no_key_material(self) -> None: + """A key readable here would widen a platform key into the ability to + execute a deployment. + """ + response = self._get(str(self._make_key().key)) + + (listed,) = response.json()["results"] + self.assertEqual( + [field for field in listed if "key" in field.lower()], + [], + f"the listing published key material: {sorted(listed)}", + ) + + def test_a_read_only_key_may_list(self) -> None: + key = self._make_key(permission=ApiKeyPermission.READ) + + self.assertEqual(self._get(str(key.key)).status_code, 200) + + def test_an_organisation_with_nothing_deployed_lists_nothing(self) -> None: + key = self._make_key(organization=self.org_b) + + response = self._get(str(key.key), organization_id=ORG_B) + + self.assertEqual(response.status_code, 200) + self.assertEqual(response.json()["count"], 0) + + def test_the_listing_is_scoped_to_the_organisation_not_the_installation( + self, + ) -> None: + """Two organisations, one deployment each: a key sees its own.""" + self._make_deployment(self.org_b, api_name="receipts") + key = self._make_key(organization=self.org_b) + + response = self._get(str(key.key), organization_id=ORG_B) + + self.assertEqual( + [listed["api_name"] for listed in response.json()["results"]], ["receipts"] + ) + + def test_the_run_summary_matches_what_has_run(self) -> None: + """The counts come from annotations on the list query rather than from + a query per row, so they are worth reading back. + """ + key = str(self._make_key().key) + + (listed,) = self._get(key).json()["results"] + self.assertEqual(listed["run_count"], 0) + self.assertIsNone(listed["last_run_time"]) + + # Written without `save()`: the model's post-save hooks reach Redis, + # which has nothing to do with what is being read back here. + (execution,) = WorkflowExecution.objects.bulk_create( + [ + WorkflowExecution( + pipeline_id=self.deployment.id, workflow=self.deployment.workflow + ) + ] + ) + + (listed,) = self._get(key).json()["results"] + self.assertEqual(listed["run_count"], 1) + self.assertEqual(listed["last_run_time"], execution.created_at.isoformat()) + + # The same serializer over a plain row, as `by_prompt_studio_tool` + # serializes one: no annotations, same answer. + unannotated = APIDeploymentListSerializer(self.deployment).data + self.assertEqual(unannotated["run_count"], 1) + self.assertEqual(unannotated["last_run_time"], execution.created_at.isoformat()) + + def test_a_never_run_deployment_costs_no_extra_summary_query(self) -> None: + """Its run annotations come back `None`, which still counts as annotated: + reading the value rather than its presence sends every never-run row + back to the database. + """ + key = str(self._make_key().key) + + def executions(queries) -> int: + return len( + [query for query in queries if "workflow_execution" in query["sql"]] + ) + + with CaptureQueriesContext(connection) as one_row: + self._get(key) + for name in ("receipts", "contracts"): + self._make_deployment(self.org_a, api_name=name) + with CaptureQueriesContext(connection) as three_rows: + self._get(key) + + # One per added row, for `last_5_run_statuses`, which is not annotated. + self.assertEqual(executions(three_rows) - executions(one_row), 2) + + def test_paging_reaches_every_deployment_exactly_once(self) -> None: + """Deployments that have never run all tie on the primary ordering, and + each page is its own query: without a unique tie-breaker a page can + repeat a row and drop another. + """ + for name in ("receipts", "contracts"): + self._make_deployment(self.org_a, api_name=name) + key = str(self._make_key().key) + + seen = [] + for page in (1, 2, 3): + response = self.client.get( + f"{listing_url(ORG_A)}?page={page}&page_size=1", + HTTP_AUTHORIZATION=f"Bearer {key}", + ) + self.assertEqual(response.status_code, 200) + seen += [listed["id"] for listed in response.json()["results"]] + + self.assertEqual(sorted(seen), sorted(set(seen))) + self.assertEqual(len(seen), 3) + + def test_a_malformed_workflow_filter_is_a_bad_request(self) -> None: + """Django raises on evaluation, which is past the point where a bad + request can still be answered as one. + """ + key = str(self._make_key().key) + + response = self.client.get( + f"{listing_url(ORG_A)}?workflow=not-a-uuid", + HTTP_AUTHORIZATION=f"Bearer {key}", + ) + + self.assertEqual(response.status_code, 400) + + def test_the_workflow_filter_selects_by_workflow(self) -> None: + other = self._make_deployment(self.org_a, api_name="receipts") + key = str(self._make_key().key) + + response = self.client.get( + f"{listing_url(ORG_A)}?workflow={other.workflow_id}", + HTTP_AUTHORIZATION=f"Bearer {key}", + ) + + self.assertEqual( + [listed["api_name"] for listed in response.json()["results"]], ["receipts"] + ) + + def test_a_request_without_a_key_is_refused(self) -> None: + self.assertEqual(self._get().status_code, 401) + + def test_an_unknown_key_is_refused(self) -> None: + self.assertEqual(self._get(str(uuid.uuid4())).status_code, 401) + + def test_a_key_cannot_list_another_organisation(self) -> None: + """The organisation is named in the path, so this is what stops a key + reading across the installation. + """ + key = self._make_key(organization=self.org_a) + + self.assertEqual(self._get(str(key.key), organization_id=ORG_B).status_code, 403) diff --git a/backend/api_v2/tests/test_docstudio_spec.py b/backend/api_v2/tests/test_docstudio_spec.py index c8b2a1d8c2..e528c5e979 100644 --- a/backend/api_v2/tests/test_docstudio_spec.py +++ b/backend/api_v2/tests/test_docstudio_spec.py @@ -22,12 +22,15 @@ from platform_api.models import ApiKeyPermission from rest_framework.exceptions import APIException, ValidationError from rest_framework.test import APIRequestFactory +from utils.user_context import UserContext from workflow_manager.endpoint_v2.dto import FileExecutionResult from workflow_manager.workflow_v2.dto import ExecutionResponse +from api_v2.api_deployment_views import APIDeploymentViewSet from api_v2.management.commands.generate_docstudio_spec import ( DEFAULT_OUT, DOWNSTREAM, + ORG_SEGMENT, REGENERATE, SpecGenerationFailed, render_spec, @@ -60,6 +63,24 @@ #: fail differently, so several checks below split on this. DEPLOYMENT_OPERATIONS = {"execute", "status"} +#: The operations a caller can address wrongly, because they take a body or a +#: parameter. The rest cannot answer 400 whatever the caller sends. +REJECTABLE_REQUEST_OPERATIONS = DEPLOYMENT_OPERATIONS | {"list_deployments"} + +#: The operations that can refuse a credential they recognise -- a key for +#: another deployment, or for another organisation. Where a key that resolves +#: at all is a key that may proceed, there is no 403 to document. +REFUSABLE_OPERATIONS = DEPLOYMENT_OPERATIONS | {"list_deployments"} + + +@pytest.fixture(autouse=True) +def _outside_any_request() -> None: + """Generation reaches the organisation-scoped managers, and the command + runs with nothing in this thread-local. A value left by an earlier test + sends them to a database these tests do not open. + """ + UserContext.set_organization_identifier(None) + def _committed() -> dict: return json.loads(DEFAULT_OUT.read_text()) @@ -132,6 +153,16 @@ def off_prefix_generator(self, request=None, public=False) -> dict: render_spec() +def _routed(path: str) -> str: + """The path Django's URLconf sees, given a documented one. + + They differ by the organisation segment, which `OrganizationMiddleware` + strips before routing. + """ + concrete = path.replace("{org_name}", "ORG").replace("{api_name}", "API") + return concrete.replace(f"/{ORG_SEGMENT}/", "/") + + def test_spec_paths_are_the_urls_the_server_serves() -> None: """Resolves the real mount rather than restating it: a spec generated for URLs the server does not serve is the failure this file exists to catch. @@ -139,10 +170,7 @@ def test_spec_paths_are_the_urls_the_server_serves() -> None: served = reverse( "api_deployment_execution", kwargs={"org_name": "ORG", "api_name": "API"} ) - documented = [ - path.replace("{org_name}", "ORG").replace("{api_name}", "API") - for path in _committed()["paths"] - ] + documented = [_routed(path) for path in _committed()["paths"]] assert served.rstrip("/") in [path.rstrip("/") for path in documented] for path in documented: @@ -150,6 +178,74 @@ def test_spec_paths_are_the_urls_the_server_serves() -> None: resolve(path if path.endswith("/") else f"{path}/") +def test_the_listing_is_documented_at_the_url_the_server_serves() -> None: + """The listing's mount is restated in `deployment_spec_urls` rather than + selected, so nothing but this holds it to the route it stands for. + """ + (documented,) = ( + path + for path, _, operation in _operations(_committed()) + if operation["operationId"] == "list_deployments" + ) + + assert _routed(documented) == reverse("tenant:api_deployment") + + +def test_the_listing_documents_only_the_method_it_publishes() -> None: + """The served route also answers POST to create a deployment, which is not + published; the restated route names one method, and this says which. + """ + served = resolve(reverse("tenant:api_deployment")) + assert served.func.cls is APIDeploymentViewSet + assert served.func.actions["get"] == APIDeploymentViewSet.list.__name__ + + (path,) = ( + path + for path, _, operation in _operations(_committed()) + if operation["operationId"] == "list_deployments" + ) + assert set(_committed()["paths"][path]) & set(_METHODS) == {"get"} + + +def test_the_listing_asks_for_the_organisation_it_lists() -> None: + """The counterpart of `test_the_identity_read_asks_for_no_organisation`. + The router never sees this segment, so nothing but generation puts it in + the spec. + """ + reads = [ + (path, operation) + for path, _, operation in _operations(_committed()) + if operation["operationId"] == "list_deployments" + ] + + assert reads + for path, operation in reads: + assert ORG_SEGMENT in path, path + declared = [ + parameter + for parameter in operation["parameters"] + if parameter["in"] == "path" + ] + assert [parameter["name"] for parameter in declared] == ["org_id"], path + assert declared[0]["required"] is True, path + + +def test_the_listed_fields_a_client_reads_are_not_optional() -> None: + """The model defaults these, so DRF reports them optional for a request + body. In a response they are always sent, and a client that types them + nullable makes every caller check a key that is always there. + """ + listed = _schema("APIDeploymentSummary") + + assert { + "api_name", + "api_endpoint", + "display_name", + "description", + "is_active", + } <= set(listed["required"]) + + def test_spec_documents_the_deployment_operations() -> None: spec = _committed() documented = {operation["operationId"] for _, _, operation in _operations(spec)} @@ -205,19 +301,25 @@ def test_clients_can_branch_on_every_failure_they_will_see() -> None: assert {"401", "500"} <= set(operation["responses"]), f"{method} {path}" -def test_the_deployment_operations_document_a_rejected_request_and_a_missing_one() -> ( - None -): - """Kept off the universal check above: a request carrying no body and - naming no resource cannot be malformed or miss its target, and documenting - a status an operation cannot return hands clients a dead branch. +def test_operations_document_a_rejected_request_only_where_one_is_possible() -> None: + """Kept off the universal check above: a request carrying no body and no + parameter cannot be malformed, not every credential can be refused once it + is recognised, nothing but the deployment operations names a resource that + can be missing, and documenting a status an operation cannot return hands + clients a dead branch. """ for path, method, operation in _operations(_committed()): - declared = {"400", "403", "404"} & set(operation["responses"]) - if operation["operationId"] in DEPLOYMENT_OPERATIONS: - assert declared == {"400", "403", "404"}, f"{method} {path}" - else: - assert not declared, f"{method} {path}" + responses = set(operation["responses"]) + operation_id = operation["operationId"] + assert ("400" in responses) is ( + operation_id in REJECTABLE_REQUEST_OPERATIONS + ), f"{method} {path}" + assert ("403" in responses) is ( + operation_id in REFUSABLE_OPERATIONS + ), f"{method} {path}" + assert ("404" in responses) is ( + operation_id in DEPLOYMENT_OPERATIONS + ), f"{method} {path}" def test_only_the_execution_endpoint_documents_the_statuses_only_it_returns() -> None: @@ -253,6 +355,50 @@ def test_the_one_shot_read_is_documented_where_a_client_will_see_it() -> None: assert status_op["responses"]["406"]["description"].strip() +# What each description asserts about how the server behaves. Prose that makes +# a promise is contract text, and reaches the caller as the client's docstring +# and the CLI's help, so it is pinned like any other part of the contract. +BEHAVIOUR_PROMISED_IN_PROSE = { + "whoami": ["no organisation segment", "rejected as unauthenticated"], + "list_deployments": ["not part of this listing", "most recent run first"], + "execute": [ + "global API deployment key", + "carrying neither is rejected", + "`timeout` of -1", + ], + "status": ["one-shot", "422"], +} + + +def test_every_operation_carries_a_summary_a_command_list_can_show() -> None: + """A generated client headlines its method with the summary, and a CLI + built from this spec lists each command by it. Without one the listing + falls back to the operation id, or to nothing at all. + """ + for path, method, operation in _operations(_committed()): + summary = operation.get("summary", "") + assert summary, f"{method} {path}" + # Long enough to say something, short enough for one terminal row. + assert 20 <= len(summary) <= 60, f"{method} {path}: {summary}" + assert not summary.endswith("."), f"{method} {path}: {summary}" + assert summary != operation["description"], f"{method} {path}" + + +def test_each_description_still_promises_what_it_promised() -> None: + """These sentences are the only place a caller learns the behaviour they + describe, and nothing else fails when one is edited away. + """ + described = { + operation["operationId"]: operation["description"] + for _, _, operation in _operations(_committed()) + } + + assert set(described) == set(BEHAVIOUR_PROMISED_IN_PROSE) + for operation_id, promises in BEHAVIOUR_PROMISED_IN_PROSE.items(): + for promise in promises: + assert promise in described[operation_id], f"{operation_id}: {promise}" + + def test_documents_are_uploaded_as_binary_not_as_urls() -> None: """A bare DRF FileField documents as `format: uri`, which generators turn into a string parameter and no multipart upload. diff --git a/backend/backend/settings/base.py b/backend/backend/settings/base.py index 988c91465e..1a923bc9fc 100644 --- a/backend/backend/settings/base.py +++ b/backend/backend/settings/base.py @@ -671,7 +671,10 @@ def filter(self, record): "deploymentKey": { "type": "http", "scheme": "bearer", - "description": "The API deployment's own key.", + "description": ( + "A key that runs an API deployment: the deployment's own " + "key, or a global API deployment key with access to it." + ), }, "platformKey": { "type": "http", @@ -696,8 +699,8 @@ def filter(self, record): { "name": "deployment", "description": ( - "Run an API deployment against one or more documents and poll " - "the result." + "Discover an organisation's API deployments, run one against " + "one or more documents, and poll the result." ), }, { diff --git a/backend/platform_api/openapi_schema.py b/backend/platform_api/openapi_schema.py index 3716da13e9..6970237fb0 100644 --- a/backend/platform_api/openapi_schema.py +++ b/backend/platform_api/openapi_schema.py @@ -98,22 +98,14 @@ class PlatformKeyError(serializers.Serializer): message = serializers.CharField(help_text="Human-readable reason for the refusal.") +WHOAMI_SUMMARY = "Resolve the organisation a platform key belongs to" + WHOAMI_DESCRIPTION = ( - "Resolve the organisation a platform API key belongs to.\n\n" - "The organisation is read from the key itself, so this route carries no " - "organisation segment and needs nothing but the key. Call it once and store " - "`organization_id`; every other endpoint takes it as a path segment.\n\n" - "Only a platform API key is accepted. An API deployment key authenticates " - "against a different table on a path that never reaches this endpoint, and " - "is rejected as unauthenticated.\n\n" - "This route serves GET only. Another method is refused either by the " - "key's permission tier or by the route itself; neither refusal is " - "described here, because OpenAPI attaches responses to an operation and " - "there is no operation for a method the route does not serve.\n\n" - "The same route also answers under an organisation segment " - "(`/api/v1/unstract/{org}/whoami/`), where the key must additionally belong " - "to the organisation named. Prefer the form documented here: it is the one " - "that needs no organisation to begin with." + "Takes a platform API key and nothing else: the organisation is read from " + "the key itself, so this route carries no organisation segment. Call it " + "once and store `organization_id`; every other endpoint takes it as a path " + "segment.\n\n" + "An API deployment key is rejected as unauthenticated." ) @@ -122,6 +114,7 @@ class PlatformKeyError(serializers.Serializer): WHOAMI_SCHEMA = extend_schema_view( get=extend_schema( operation_id="whoami", + summary=WHOAMI_SUMMARY, tags=["identity"], auth=[{"platformKey": []}], responses={ diff --git a/specs/docstudio-oss.json b/specs/docstudio-oss.json index 345b802cf1..7bd536cda3 100644 --- a/specs/docstudio-oss.json +++ b/specs/docstudio-oss.json @@ -1,6 +1,102 @@ { "components": { "schemas": { + "APIDeploymentSummary": { + "description": "One API deployment, as it appears in an organisation's listing.\n\n`api_name` and `api_endpoint` are what an execution call needs;\n`display_name` and `description` say what the deployment is for.", + "properties": { + "api_endpoint": { + "readOnly": true, + "type": "string" + }, + "api_name": { + "maxLength": 30, + "readOnly": true, + "type": "string" + }, + "co_owners_count": { + "readOnly": true, + "type": "integer" + }, + "created_by": { + "nullable": true, + "readOnly": true, + "type": "integer" + }, + "created_by_email": { + "description": "Get the email of the creator.", + "nullable": true, + "readOnly": true, + "type": "string" + }, + "description": { + "maxLength": 255, + "readOnly": true, + "type": "string" + }, + "display_name": { + "maxLength": 30, + "readOnly": true, + "type": "string" + }, + "id": { + "format": "uuid", + "readOnly": true, + "type": "string" + }, + "is_active": { + "readOnly": true, + "type": "boolean" + }, + "is_owner": { + "readOnly": true, + "type": "boolean" + }, + "last_5_run_statuses": { + "description": "Fetch the last 5 execution statuses with timestamps for this API deployment.", + "items": { + "additionalProperties": {}, + "type": "object" + }, + "readOnly": true, + "type": "array" + }, + "last_run_time": { + "nullable": true, + "readOnly": true, + "type": "string" + }, + "run_count": { + "readOnly": true, + "type": "integer" + }, + "workflow": { + "format": "uuid", + "type": "string" + }, + "workflow_name": { + "readOnly": true, + "type": "string" + } + }, + "required": [ + "api_endpoint", + "api_name", + "co_owners_count", + "created_by", + "created_by_email", + "description", + "display_name", + "id", + "is_active", + "is_owner", + "last_5_run_statuses", + "last_run_time", + "run_count", + "workflow", + "workflow_name" + ], + "type": "object" + }, "AcknowledgedResponse": { "description": "The execution's result was handed to an earlier call and discarded.", "properties": { @@ -218,6 +314,37 @@ ], "type": "object" }, + "PaginatedAPIDeploymentSummaryList": { + "properties": { + "count": { + "example": 123, + "type": "integer" + }, + "next": { + "example": "http://api.example.org/accounts/?page=4", + "format": "uri", + "nullable": true, + "type": "string" + }, + "previous": { + "example": "http://api.example.org/accounts/?page=2", + "format": "uri", + "nullable": true, + "type": "string" + }, + "results": { + "items": { + "$ref": "#/components/schemas/APIDeploymentSummary" + }, + "type": "array" + } + }, + "required": [ + "count", + "results" + ], + "type": "object" + }, "PlatformKeyError": { "description": "Why a platform-key request was refused.\n\nProduced by the authentication middleware rather than by the project's\nexception handler, so it carries a single human-readable message and none\nof the per-field structure the organisation-scoped endpoints return. It is\nthe shape of this operation's credential failures specifically, not of\nevery failure it can return.", "properties": { @@ -284,7 +411,7 @@ }, "securitySchemes": { "deploymentKey": { - "description": "The API deployment's own key.", + "description": "A key that runs an API deployment: the deployment's own key, or a global API deployment key with access to it.", "scheme": "bearer", "type": "http" }, @@ -303,7 +430,7 @@ "paths": { "/api/v1/unstract/whoami/": { "get": { - "description": "Resolve the organisation a platform API key belongs to.\n\nThe organisation is read from the key itself, so this route carries no organisation segment and needs nothing but the key. Call it once and store `organization_id`; every other endpoint takes it as a path segment.\n\nOnly a platform API key is accepted. An API deployment key authenticates against a different table on a path that never reaches this endpoint, and is rejected as unauthenticated.\n\nThis route serves GET only. Another method is refused either by the key's permission tier or by the route itself; neither refusal is described here, because OpenAPI attaches responses to an operation and there is no operation for a method the route does not serve.\n\nThe same route also answers under an organisation segment (`/api/v1/unstract/{org}/whoami/`), where the key must additionally belong to the organisation named. Prefer the form documented here: it is the one that needs no organisation to begin with.", + "description": "Takes a platform API key and nothing else: the organisation is read from the key itself, so this route carries no organisation segment. Call it once and store `organization_id`; every other endpoint takes it as a path segment.\n\nAn API deployment key is rejected as unauthenticated.", "operationId": "whoami", "responses": { "200": { @@ -335,14 +462,138 @@ "platformKey": [] } ], + "summary": "Resolve the organisation a platform key belongs to", "tags": [ "identity" ] } }, + "/api/v1/unstract/{org_id}/api/deployment/": { + "get": { + "description": "Lists what an organisation has deployed, authenticated by a platform API key that belongs to it.\n\nA deployment key is not part of this listing; executing a deployment still needs one.\n\nOrdered by most recent run first, then by identifier, so paging is stable.", + "operationId": "list_deployments", + "parameters": [ + { + "description": "Return only the deployment with exactly this `api_name`.", + "in": "query", + "name": "api_name", + "schema": { + "type": "string" + } + }, + { + "description": "Which field to use when ordering the results.", + "in": "query", + "name": "ordering", + "required": false, + "schema": { + "type": "string" + } + }, + { + "description": "A page number within the paginated result set.", + "in": "query", + "name": "page", + "required": false, + "schema": { + "type": "integer" + } + }, + { + "description": "Number of results to return per page.", + "in": "query", + "name": "page_size", + "required": false, + "schema": { + "type": "integer" + } + }, + { + "description": "Return only deployments whose display name contains this text, case-insensitively.", + "in": "query", + "name": "search", + "schema": { + "type": "string" + } + }, + { + "description": "Return only the deployments of this workflow.", + "in": "query", + "name": "workflow", + "schema": { + "format": "uuid", + "type": "string" + } + }, + { + "description": "The organisation the request is scoped to, as `whoami` reports it in `organization_id`.", + "in": "path", + "name": "org_id", + "required": true, + "schema": { + "type": "string" + } + } + ], + "responses": { + "200": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/PaginatedAPIDeploymentSummaryList" + } + } + }, + "description": "One page of the organisation's API deployments." + }, + "400": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorResponse" + } + } + }, + "description": "A query parameter was malformed." + }, + "401": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/PlatformKeyError" + } + } + }, + "description": "No usable platform API key was supplied \u2014 absent, malformed, unknown, or revoked." + }, + "403": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/PlatformKeyError" + } + } + }, + "description": "The key was recognised but refused: it does not belong to the organisation named in the path, or its permission tier is not one this deployment knows." + }, + "500": { + "description": "The request could not be served. The body is not guaranteed to be JSON." + } + }, + "security": [ + { + "platformKey": [] + } + ], + "summary": "List an organisation's API deployments", + "tags": [ + "deployment" + ] + } + }, "/deployment/api/{org_name}/{api_name}/": { "get": { - "description": "Read the result of a previously started execution.\n\nThis read is one-shot: the first call that observes a completed execution acknowledges it and the stored result is discarded, so every later call for that execution answers 406. Poll while the execution is pending, and keep the payload of the call that returns it \u2014 it cannot be fetched again.\n\nA still-running execution answers 422 carrying its current `status`, so a polling loop should treat 422 as the normal reply and stop on 200. Clients that raise on any non-2xx need to allow for that.", + "description": "Reads a previously started execution, taking the same deployment key that ran it. The read is one-shot: the first call that observes a completed execution acknowledges it and the stored result is discarded, so every later call for that execution answers 406. Keep the payload of the call that returns it \u2014 it cannot be fetched again.\n\nA still-running execution answers 422 carrying its current `status`, so a polling loop should treat 422 as the normal reply and stop on 200.", "operationId": "status", "parameters": [ { @@ -568,12 +819,13 @@ "deploymentKey": [] } ], + "summary": "Read the result of an execution", "tags": [ "deployment" ] }, "post": { - "description": "Execute an API deployment against one or more documents.\n\nSupply the documents either as `files` (multipart upload) or as `presigned_urls` (HTTPS S3 URLs), or both \u2014 a request carrying neither is rejected, and the two together may not exceed 32 documents.\n\nWith the default `timeout` of -1 the call returns as soon as the execution is queued; read the outcome from the status endpoint.", + "description": "Runs an API deployment. Takes a deployment key \u2014 either the deployment's own key, or a global API deployment key that has access to it.\n\nSupply the documents either as `files` (multipart upload) or as `presigned_urls` (HTTPS S3 URLs), or both \u2014 a request carrying neither is rejected, and the two together may not exceed 32 documents.\n\nWith the default `timeout` of -1 the call returns as soon as the execution is queued; read the outcome from the status endpoint.", "operationId": "execute", "parameters": [ { @@ -805,6 +1057,7 @@ "deploymentKey": [] } ], + "summary": "Execute an API deployment against documents", "tags": [ "deployment" ] @@ -813,7 +1066,7 @@ }, "tags": [ { - "description": "Run an API deployment against one or more documents and poll the result.", + "description": "Discover an organisation's API deployments, run one against one or more documents, and poll the result.", "name": "deployment" }, {