From 7b01720ad65f7c23ae84c34b94f848834489ff70 Mon Sep 17 00:00:00 2001 From: ATC964 <82515918+atc964@users.noreply.github.com> Date: Thu, 17 Sep 2026 18:27:02 -0400 Subject: [PATCH 1/2] fix: stop the unauthenticated change-request listing from returning every order's audit trail (seller-928) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GET /api/v1/change-requests took optional auth, required no filter, and declared no response_model. One anonymous request returned every change request for every order, and each record carries rollback_snapshot — a full copy of the order including its state-machine audit log, every transition's actor and reason, plus the quote id, deal id and metadata. Two gates close it. Create, list and read-by-id now take require_api_key_record (anonymous -> 401), matching the approvals router; review and apply keep their operator gate. Authentication alone is not enough, so each record is stamped at creation with an actor derived from the credential and buyer callers see only their own records — another actor's record reads 404, not 403, so the by-id route is not an existence oracle. Operator callers stay unscoped because they review the queue. All five handlers declare a response_model, so rollback_snapshot cannot reach the wire at all; it is still written server-side (deleting it is tracked separately). requested_by is dropped from the create body and stamped from the credential instead — it is the field a reviewer trusts when approving, so a requester must not be able to name itself. The change-request idempotency key is namespaced per actor as well as per order, closing the replay path that would otherwise hand one buyer another buyer's record straight past the scoping. The list scope is applied in the service in Python rather than pushed into the storage filters dict, because StorageBackend.list_change_requests understands only order_id and status and silently ignores any other key — a scope passed that way would fail open. --- CHANGELOG.md | 19 + docs/api/change-requests.md | 34 +- docs/api/overview.md | 12 +- docs/state-machines/change-request-flow.md | 4 +- src/ad_seller/auth/dependencies.py | 37 ++ .../interfaces/api/routers/change_requests.py | 129 ++++- src/ad_seller/interfaces/api/schemas.py | 61 ++- src/ad_seller/services/order_service.py | 40 +- tests/unit/test_change_request.py | 43 +- tests/unit/test_change_request_disclosure.py | 517 ++++++++++++++++++ tests/unit/test_operator_auth.py | 25 +- 11 files changed, 863 insertions(+), 58 deletions(-) create mode 100644 tests/unit/test_change_request_disclosure.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 4a408c9a..239e4891 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,25 @@ All notable changes to the IAB Tech Lab Seller Agent are documented here. orphaned worker thread (CrewAI has no cancellation API). Budget <= 0 still means "no bound" and always runs the crew; wire answers on the timeout path are unchanged. +- **Breaking — change-request routes now require a credential and are + scoped to the caller (seller-928).** `GET /api/v1/change-requests`, + `GET /api/v1/change-requests/{cr_id}` and + `POST /api/v1/change-requests` reject anonymous callers with `401`; a + buyer credential sees only its own change requests (another actor's + record reads `404`, not `403`), while an operator credential still sees + the whole queue for review. All five handlers now declare a + `response_model`, so `rollback_snapshot` — a full copy of the order + including its state-machine audit log, quote id and deal id — can no + longer reach the wire; it is still recorded server-side. `requested_by` + has been removed from the create body and is stamped from the presented + credential instead; a body that still sends it is accepted and the + value ignored. The idempotency key for change-request creation is now + namespaced per actor as well as per order. + **Operators on an earlier build should assume every order's change + requests — audit trail, transition actors and reasons, quote ids and + deal ids — were readable by anyone who could reach the seller, and that + any `requested_by` value in a record created on that build is + self-asserted and not trustworthy.** ### Fixed diff --git a/docs/api/change-requests.md b/docs/api/change-requests.md index 3d15b7a3..20957a51 100644 --- a/docs/api/change-requests.md +++ b/docs/api/change-requests.md @@ -2,6 +2,19 @@ Change requests handle post-deal modifications to orders. Each request is validated against the current order state, assigned a severity level, and routed through the appropriate approval path. +## Authentication and scoping + +**Every route on this surface requires a credential.** Anonymous callers get `401`. + +- **Submission and reads** (`POST /api/v1/change-requests`, `GET /api/v1/change-requests`, `GET /api/v1/change-requests/{cr_id}`) accept a **buyer** key. +- **Review and apply** require an **operator** key (buyer key → `403`). + +Reads are additionally **scoped to the caller**. A buyer credential sees only the change requests it filed; another actor's record reads `404` (not `403`, so the by-id route cannot be walked as an existence oracle). An operator credential sees the whole queue — that is who reviews it. + +The requesting actor is **stamped by the seller** from the presented credential and is stable across key rotation for a buyer with a seat/agency/advertiser identity. It is never taken from the request body: `requested_by` is the field a reviewer trusts when approving, so a requester must not be able to name itself. + +Change request records embed a server-side `rollback_snapshot` (a full copy of the order, audit log included). It is not part of any response on this surface and never crosses the wire. + ## Severity Levels Severity is auto-classified based on change type and magnitude: @@ -38,12 +51,14 @@ Severity is auto-classified based on change type and magnitude: | `diffs` | array | No | List of field-level changes: `{field, old_value, new_value}` | | `proposed_values` | object | No | Key-value pairs of proposed new values | | `reason` | string | No | Explanation for the change | -| `requested_by` | string | No | Who requested (default: `system`) | + +`requested_by` is **not** a request field. It is derived from the presented credential. A body that still sends it is accepted and the value ignored. ### Example: Shift Flight Dates (Minor --- Auto-Approved) ```bash curl -X POST http://localhost:8000/api/v1/change-requests \ + -H "Authorization: Bearer " \ -H "Content-Type: application/json" \ -d '{ "idempotency_key": "", @@ -53,8 +68,7 @@ curl -X POST http://localhost:8000/api/v1/change-requests \ {"field": "flight_start", "old_value": "2026-04-01", "new_value": "2026-04-03"}, {"field": "flight_end", "old_value": "2026-04-30", "new_value": "2026-05-02"} ], - "reason": "Campaign launch delayed by 2 days", - "requested_by": "agent:buyer-001" + "reason": "Campaign launch delayed by 2 days" }' ``` @@ -64,6 +78,7 @@ Because the date shift is 2 days (within the 3-day threshold), this is classifie ```bash curl -X POST http://localhost:8000/api/v1/change-requests \ + -H "Authorization: Bearer " \ -H "Content-Type: application/json" \ -d '{ "idempotency_key": "", @@ -73,8 +88,7 @@ curl -X POST http://localhost:8000/api/v1/change-requests \ {"field": "final_cpm", "old_value": 10.00, "new_value": 8.50} ], "proposed_values": {"final_cpm": 8.50}, - "reason": "Buyer requested discount after volume commitment", - "requested_by": "human:ops-jane" + "reason": "Buyer requested discount after volume commitment" }' ``` @@ -100,17 +114,23 @@ Failed validation returns HTTP 422 with the change request ID and error list. | `status` | string | Filter by status | ```bash -curl "http://localhost:8000/api/v1/change-requests?order_id=ORD-A1B2C3D4E5F6&status=pending_approval" +curl -H "Authorization: Bearer " \ + "http://localhost:8000/api/v1/change-requests?order_id=ORD-A1B2C3D4E5F6&status=pending_approval" ``` +A buyer credential sees only its own change requests; an operator credential sees all of them. + ## Get Change Request **GET** `/api/v1/change-requests/{cr_id}` ```bash -curl http://localhost:8000/api/v1/change-requests/CR-A1B2C3D4E5F6 +curl -H "Authorization: Bearer " \ + http://localhost:8000/api/v1/change-requests/CR-A1B2C3D4E5F6 ``` +Returns **404** for a change request filed by a different actor. + ## Review a Change Request **POST** `/api/v1/change-requests/{cr_id}/review` diff --git a/docs/api/overview.md b/docs/api/overview.md index 1c229f45..f875a819 100644 --- a/docs/api/overview.md +++ b/docs/api/overview.md @@ -160,13 +160,15 @@ Registry reads are public; mutations require an operator credential. ## Change Requests +Every route here requires a credential (anonymous → 401). Reads are scoped to the caller's own change requests; an operator credential sees the whole queue. + | Method | Path | Summary | |--------|------|---------| -| POST | `/api/v1/change-requests` | Submit a change request for an existing order | -| GET | `/api/v1/change-requests` | List change requests, optionally filtered by order or status | -| GET | `/api/v1/change-requests/{cr_id}` | Get a change request by ID | -| POST | `/api/v1/change-requests/{cr_id}/review` | Approve or reject a pending change request | -| POST | `/api/v1/change-requests/{cr_id}/apply` | Apply an approved change request to the order | +| POST | `/api/v1/change-requests` | Submit a change request for an existing order (auth required; actor stamped from the credential) | +| GET | `/api/v1/change-requests` | List the caller's change requests, optionally filtered by order or status (auth required) | +| GET | `/api/v1/change-requests/{cr_id}` | Get a change request by ID (auth required; another actor's record reads 404) | +| POST | `/api/v1/change-requests/{cr_id}/review` | Approve or reject a pending change request (operator auth required) | +| POST | `/api/v1/change-requests/{cr_id}/apply` | Apply an approved change request to the order (operator auth required) | ## Audit diff --git a/docs/state-machines/change-request-flow.md b/docs/state-machines/change-request-flow.md index 7758aab6..b59cafda 100644 --- a/docs/state-machines/change-request-flow.md +++ b/docs/state-machines/change-request-flow.md @@ -93,7 +93,7 @@ The `ChangeRequest` model tracks the full lifecycle: | `status` | ChangeRequestStatus | Current lifecycle status | | `change_type` | ChangeType | Category of change | | `severity` | ChangeSeverity | `minor`, `material`, or `critical` | -| `requested_by` | string | Who requested the change | +| `requested_by` | string | Who requested the change. Stamped by the seller from the presented credential, never taken from the request body | | `requested_at` | datetime | When the request was created | | `reason` | string | Explanation for the change | | `diffs` | list[FieldDiff] | Field-level changes: `{field, old_value, new_value}` | @@ -106,4 +106,4 @@ The `ChangeRequest` model tracks the full lifecycle: | `rejection_reason` | string | Reason for rejection | | `applied_at` | datetime | When changes were applied | | `applied_by` | string | Who applied the changes | -| `rollback_snapshot` | dict | Snapshot of order state before changes (for rollback) | +| `rollback_snapshot` | dict | Snapshot of order state before changes (for rollback). **Server-side only** — it is a full copy of the order, audit log included, and is excluded from every response on the change-request surface | diff --git a/src/ad_seller/auth/dependencies.py b/src/ad_seller/auth/dependencies.py index d5dd417f..bbf8ae66 100644 --- a/src/ad_seller/auth/dependencies.py +++ b/src/ad_seller/auth/dependencies.py @@ -161,3 +161,40 @@ def principal_from_api_key(record: ApiKeyRecord) -> str: or "unknown" ) return f"apikey:{record.key_id}:{buyer}" + + +def actor_from_api_key(record: ApiKeyRecord) -> str: + """Derive the seller-stamped ACTOR id for a credentialed request. + + Unlike :func:`principal_from_api_key` — which anchors to the immutable + ``key_id`` because an approval audit entry must name the exact + credential that decided — this is an OWNERSHIP key: it must stay the + same for the same buyer across key rotation, or a buyer would lose + sight of records it filed under an earlier key. So it anchors to the + strongest stable buyer identifier and only falls back to ``key_id`` + for a credential that asserts no buyer identity at all (where no + stable owner exists to anchor to). + + Shape follows the shared contract's actor convention + (``'system'`` / ``'human:'`` / ``'agent:'``): a buyer-agent + credential is an ``agent:``; an operator credential gets its own + ``operator:`` namespace so a buyer identifier can never collide with + a seller-side actor. + + Used to stamp ``ChangeRequest.requested_by`` and to scope change + request reads — it is never taken from the wire. + """ + from ..models.api_key import ApiKeyRole + + if record.role == ApiKeyRole.OPERATOR: + return f"operator:{record.key_id}" + + identity = record.identity + buyer = ( + getattr(identity, "advertiser_id", None) + or getattr(identity, "agency_id", None) + or getattr(identity, "seat_id", None) + ) + if buyer: + return f"agent:{buyer}" + return f"agent:key:{record.key_id}" diff --git a/src/ad_seller/interfaces/api/routers/change_requests.py b/src/ad_seller/interfaces/api/routers/change_requests.py index 9a00a15e..63c00fc8 100644 --- a/src/ad_seller/interfaces/api/routers/change_requests.py +++ b/src/ad_seller/interfaces/api/routers/change_requests.py @@ -1,39 +1,94 @@ # Author: Green Mountain Systems AI Inc. # Donated to IAB Tech Lab -"""Change request endpoints.""" +"""Change request endpoints. + +Every route here REQUIRES a credential and every route declares a +``response_model`` (seller-928). + +Why: a change request record embeds ``rollback_snapshot`` — a full copy +of the order, including its state-machine audit log with every +transition's actor and reason, plus the order's quote id, deal id and +metadata. Before this module was hardened the listing route took optional +auth, required no filter, and declared no response model, so a single +anonymous ``GET /api/v1/change-requests`` returned every order's audit +trail for every tenant. + +The two gates that close it: + +* **Authentication.** Create/list/read take ``require_api_key_record`` + (anonymous → 401), matching the approvals router. Review/apply keep + their stricter operator gate. +* **Ownership.** Authentication alone is not enough — an authenticated + buyer would still read every other buyer's trail. Each record is + stamped at creation with an actor derived from the credential + (``actor_from_api_key``), and buyer callers see only records carrying + their own actor. Operator callers are unscoped: reviewing a change + request is the seller's job and needs the whole queue. +""" from typing import Optional from fastapi import APIRouter, Depends, HTTPException +from ....auth.dependencies import actor_from_api_key, require_api_key_record +from ....models.api_key import ApiKeyRecord, ApiKeyRole from ....services import order_service from .. import contract_mappers as cm from .. import deps -from ..schemas import CreateChangeRequestModel, ReviewChangeRequestModel +from ..schemas import ( + ChangeRequestApplyResponse, + ChangeRequestListResponse, + ChangeRequestResponse, + CreateChangeRequestModel, + ReviewChangeRequestModel, +) router = APIRouter() -@router.post("/api/v1/change-requests", tags=["Change Requests"]) +def _read_scope(caller: ApiKeyRecord) -> Optional[str]: + """Actor id a caller's reads are confined to, or None for unscoped. + + Operators see the whole queue — they are the ones who review it. + Every other credential sees only what it filed. + """ + if caller.role == ApiKeyRole.OPERATOR: + return None + return actor_from_api_key(caller) + + +@router.post( + "/api/v1/change-requests", + tags=["Change Requests"], + response_model=ChangeRequestResponse, +) async def create_change_request( request: CreateChangeRequestModel, - _auth: None = Depends(deps._get_optional_api_key_record), + caller: ApiKeyRecord = Depends(require_api_key_record), ): - """Submit a change request for an existing order. + """Submit a change request for an existing order. Requires authentication. - Validates the change against the current order state, classifies - severity, and routes to approval if needed. + The requesting actor is stamped from the presented credential, never + from the body — ``requested_by`` is exactly the field a human + reviewer trusts when deciding whether to approve, so a caller must + not be able to name itself "human:cfo@publisher.example". **Idempotency (FD-12):** the request carries a required ``idempotency_key``. An identical replay returns the original change request without creating a second approval; reusing the key with a different body returns ``idempotency_conflict`` (HTTP 409). Keys are - scoped per order and expire after 24 hours. + scoped per actor and order, and expire after 24 hours. """ from ....storage.factory import get_storage - storage_key = f"idempotency:change-request:{request.order_id}:{request.idempotency_key}" + actor = actor_from_api_key(caller) + + # The actor is part of the key: without it one buyer replaying + # another's (order_id, idempotency_key) pair would be handed back the + # other buyer's change request through the replay path, straight past + # the ownership scoping below. + storage_key = f"idempotency:change-request:{actor}:{request.order_id}:{request.idempotency_key}" payload_hash = cm.request_payload_hash( request.model_dump(mode="json", exclude={"idempotency_key"}) ) @@ -52,9 +107,12 @@ async def create_change_request( "for a different change request." ), ) - return await order_service.get_change_request(prior["change_request_id"]) + return await order_service.get_change_request( + prior["change_request_id"], + requested_by=_read_scope(caller), + ) - result = await order_service.create_change_request(request) + result = await order_service.create_change_request(request, requested_by=actor) try: await storage.set( @@ -71,26 +129,51 @@ async def create_change_request( return result -@router.get("/api/v1/change-requests", tags=["Change Requests"]) +@router.get( + "/api/v1/change-requests", + tags=["Change Requests"], + response_model=ChangeRequestListResponse, +) async def list_change_requests( order_id: Optional[str] = None, status: Optional[str] = None, - _auth: None = Depends(deps._get_optional_api_key_record), + caller: ApiKeyRecord = Depends(require_api_key_record), ): - """List change requests, optionally filtered by order or status.""" - return await order_service.list_change_requests(order_id=order_id, status=status) + """List change requests, optionally filtered by order or status. + + Requires authentication and returns only the caller's own change + requests; an operator credential sees all of them. + """ + return await order_service.list_change_requests( + order_id=order_id, + status=status, + requested_by=_read_scope(caller), + ) -@router.get("/api/v1/change-requests/{cr_id}", tags=["Change Requests"]) +@router.get( + "/api/v1/change-requests/{cr_id}", + tags=["Change Requests"], + response_model=ChangeRequestResponse, +) async def get_change_request( cr_id: str, - _auth: None = Depends(deps._get_optional_api_key_record), + caller: ApiKeyRecord = Depends(require_api_key_record), ): - """Get a change request by ID.""" - return await order_service.get_change_request(cr_id) + """Get a change request by ID. + + Requires authentication. Another actor's change request reads as 404, + so the route cannot be walked as an existence oracle over change + request ids. + """ + return await order_service.get_change_request(cr_id, requested_by=_read_scope(caller)) -@router.post("/api/v1/change-requests/{cr_id}/review", tags=["Change Requests"]) +@router.post( + "/api/v1/change-requests/{cr_id}/review", + tags=["Change Requests"], + response_model=ChangeRequestResponse, +) async def review_change_request( cr_id: str, request: ReviewChangeRequestModel, @@ -109,7 +192,11 @@ async def review_change_request( ) -@router.post("/api/v1/change-requests/{cr_id}/apply", tags=["Change Requests"]) +@router.post( + "/api/v1/change-requests/{cr_id}/apply", + tags=["Change Requests"], + response_model=ChangeRequestApplyResponse, +) async def apply_change_request( cr_id: str, _operator=Depends(deps._require_operator_api_key_record), diff --git a/src/ad_seller/interfaces/api/schemas.py b/src/ad_seller/interfaces/api/schemas.py index cea4302a..346a9342 100644 --- a/src/ad_seller/interfaces/api/schemas.py +++ b/src/ad_seller/interfaces/api/schemas.py @@ -416,7 +416,15 @@ class FieldDiffModel(BaseModel): class CreateChangeRequestModel(BaseModel): - """Request to create a change request for an order.""" + """Request to create a change request for an order. + + ``requested_by`` is deliberately ABSENT. The actor is stamped + server-side from the presented credential + (``auth.dependencies.actor_from_api_key``) — it is the field a human + reviewer trusts when approving, so it must never be a wire string the + requester chooses. A body that still carries ``requested_by`` is + accepted and the value ignored (pydantic drops unknown fields). + """ idempotency_key: str = Field(min_length=1) order_id: str @@ -424,7 +432,6 @@ class CreateChangeRequestModel(BaseModel): diffs: list[FieldDiffModel] = [] proposed_values: Optional[dict] = None reason: str = "" - requested_by: str = "system" class ReviewChangeRequestModel(BaseModel): @@ -435,6 +442,56 @@ class ReviewChangeRequestModel(BaseModel): reason: str = "" +class ChangeRequestResponse(BaseModel): + """Wire shape of a change request. + + Declared explicitly so the router cannot serialise the stored record + wholesale. In particular ``rollback_snapshot`` — a full copy of the + order, including its state-machine audit log, every transition's + actor and reason, and its quote/deal ids — is a SERVER-SIDE field and + is absent here by construction, not by filtering. + """ + + change_request_id: str = "" + order_id: str = "" + deal_id: str = "" + status: str = "" + change_type: str = "" + severity: str = "" + requested_by: str = "" + requested_at: Optional[str] = None + reason: str = "" + + diffs: list[FieldDiffModel] = [] + proposed_values: dict = {} + + validation_errors: list[str] = [] + pricing_impact: Optional[dict] = None + availability_check: Optional[dict] = None + + approved_by: str = "" + approved_at: Optional[str] = None + rejection_reason: str = "" + + applied_at: Optional[str] = None + applied_by: str = "" + + +class ChangeRequestListResponse(BaseModel): + """Wire shape of ``GET /api/v1/change-requests``.""" + + change_requests: list[ChangeRequestResponse] = [] + count: int = 0 + + +class ChangeRequestApplyResponse(BaseModel): + """Wire shape of ``POST /api/v1/change-requests/{cr_id}/apply``.""" + + change_request_id: str = "" + status: str = "" + order_id: str = "" + + class DealFromTemplateRequest(BaseModel): """Request model for POST /api/v1/deals/from-template.""" diff --git a/src/ad_seller/services/order_service.py b/src/ad_seller/services/order_service.py index f850cf32..9fb14105 100644 --- a/src/ad_seller/services/order_service.py +++ b/src/ad_seller/services/order_service.py @@ -289,11 +289,17 @@ async def get_order_audit( # ============================================================================= -async def create_change_request(request: Any) -> dict[str, Any]: +async def create_change_request(request: Any, requested_by: str) -> dict[str, Any]: """Submit a change request for an existing order. Validates the change against the current order state, classifies severity, and routes to approval if needed. + + ``requested_by`` is the seller-stamped actor derived from the caller's + credential (``auth.dependencies.actor_from_api_key``). It is a required + argument, not a body field: it is both the audit identity a reviewer + trusts and the ownership key that scopes later reads, so there is no + safe default for it to fall back to. """ from ..models.change_request import ( ChangeRequest, @@ -358,7 +364,7 @@ async def create_change_request(request: Any) -> dict[str, Any]: deal_id=deal_id, change_type=change_type, severity=severity, - requested_by=request.requested_by, + requested_by=requested_by, reason=request.reason, diffs=diffs, proposed_values=request.proposed_values or {}, @@ -398,8 +404,18 @@ async def create_change_request(request: Any) -> dict[str, Any]: async def list_change_requests( order_id: Optional[str] = None, status: Optional[str] = None, + requested_by: Optional[str] = None, ) -> dict[str, Any]: - """List change requests, optionally filtered by order or status.""" + """List change requests, optionally filtered by order or status. + + ``requested_by`` scopes the listing to one actor (the caller's own + records). It is applied HERE, in Python, and not pushed into the + storage ``filters`` dict: ``StorageBackend.list_change_requests`` + understands only ``order_id`` and ``status`` and silently ignores any + other key, so a filter passed that way would fail OPEN and return + every tenant's records. ``None`` means unscoped and is reserved for + operator callers. + """ from ..storage.factory import get_storage storage = await get_storage() @@ -409,16 +425,28 @@ async def list_change_requests( if status: filters["status"] = status results = await storage.list_change_requests(filters if filters else None) + if requested_by is not None: + results = [cr for cr in results if cr.get("requested_by") == requested_by] return {"change_requests": results, "count": len(results)} -async def get_change_request(cr_id: str) -> dict[str, Any]: - """Get a change request by ID.""" +async def get_change_request( + cr_id: str, + requested_by: Optional[str] = None, +) -> dict[str, Any]: + """Get a change request by ID. + + ``requested_by`` scopes the read to one actor. A record belonging to + a different actor is reported as 404, not 403 — a distinguishable + "exists but not yours" would turn this route into an existence oracle + over every tenant's change request ids. ``None`` means unscoped and + is reserved for operator callers. + """ from ..storage.factory import get_storage storage = await get_storage() cr = await storage.get_change_request(cr_id) - if not cr: + if not cr or (requested_by is not None and cr.get("requested_by") != requested_by): raise HTTPException( status_code=404, detail={ diff --git a/tests/unit/test_change_request.py b/tests/unit/test_change_request.py index bc3e6ac5..4b724814 100644 --- a/tests/unit/test_change_request.py +++ b/tests/unit/test_change_request.py @@ -23,8 +23,11 @@ import httpx # noqa: E402 from httpx import ASGITransport # noqa: E402 +from ad_seller.auth.dependencies import require_api_key_record # noqa: E402 from ad_seller.interfaces.api import deps as api_deps # noqa: E402 from ad_seller.interfaces.api.main import _get_optional_api_key_record, app # noqa: E402 +from ad_seller.models.api_key import ApiKeyRecord # noqa: E402 +from ad_seller.models.buyer_identity import BuyerIdentity # noqa: E402 from ad_seller.models.change_request import ( # noqa: E402 ChangeRequest, ChangeSeverity, @@ -76,9 +79,24 @@ def mock_storage(): return storage +# The credential every request in this module presents. The change +# request routes require one (seller-928) and stamp the actor from it; +# these tests exercise the CR lifecycle, not auth, so one fixed buyer +# stands in for the caller throughout. Auth and ownership scoping are +# covered in test_change_request_disclosure.py and test_operator_auth.py. +TEST_CALLER = ApiKeyRecord( + key_id="key-cr-tests", + key_hash="hash-cr-tests", + key_prefix_hint="ask_live_te...", + identity=BuyerIdentity(agency_id="agy-cr-tests"), +) +TEST_ACTOR = "agent:agy-cr-tests" + + @pytest.fixture def client(mock_storage): app.dependency_overrides[_get_optional_api_key_record] = lambda: None + app.dependency_overrides[require_api_key_record] = lambda: TEST_CALLER # CR review/apply are operator-gated; these tests exercise the CR # lifecycle, not auth (covered in test_operator_auth.py). app.dependency_overrides[api_deps._require_operator_api_key_record] = lambda: None @@ -173,7 +191,7 @@ async def test_create_minor_auto_approved(self, client, mock_storage): "order_id": "ORD-TEST001", "change_type": "creative", "reason": "Swap banner creative", - "requested_by": "agent:buyer", + "requested_by": "agent:someone-else", }, ) @@ -183,6 +201,9 @@ async def test_create_minor_auto_approved(self, client, mock_storage): assert data["status"] == "approved" # auto-approved (minor) assert data["severity"] == "minor" assert data["approved_by"] == "system:auto-approve" + # The body's requested_by is ignored; the actor comes from the + # presented credential (seller-928). + assert data["requested_by"] == TEST_ACTOR async def test_create_material_needs_approval(self, client, mock_storage): _seed_order(mock_storage) @@ -203,7 +224,9 @@ async def test_create_material_needs_approval(self, client, mock_storage): assert data["status"] == "pending_approval" assert data["severity"] == "material" - async def test_create_with_rollback_snapshot(self, client, mock_storage): + async def test_rollback_snapshot_is_stored_server_side_only(self, client, mock_storage): + """The snapshot is recorded but never serialised to the wire — it is + a full copy of the order, audit log included (seller-928).""" _seed_order(mock_storage) with patch("ad_seller.storage.factory.get_storage", return_value=mock_storage): resp = await client.post( @@ -222,8 +245,10 @@ async def test_create_with_rollback_snapshot(self, client, mock_storage): }, ) - data = resp.json() - assert data["rollback_snapshot"]["order_id"] == "ORD-TEST001" + assert "rollback_snapshot" not in resp.text + cr_id = resp.json()["change_request_id"] + stored = mock_storage._store[f"change_request:{cr_id}"] + assert stored["rollback_snapshot"]["order_id"] == "ORD-TEST001" async def test_order_not_found(self, client, mock_storage): with patch("ad_seller.storage.factory.get_storage", return_value=mock_storage): @@ -382,13 +407,15 @@ async def test_idempotency_record_uses_24_hour_ttl(self, client, mock_storage): ) assert resp.status_code == 200 + # The key carries the caller's actor: without it, one buyer + # replaying another's (order_id, idempotency_key) pair would be + # handed back the other buyer's record (seller-928). + storage_key = f"idempotency:change-request:{TEST_ACTOR}:ORD-TEST001:idem-ttl-1" mock_storage.set.assert_any_await( - "idempotency:change-request:ORD-TEST001:idem-ttl-1", + storage_key, { "change_request_id": resp.json()["change_request_id"], - "payload_hash": mock_storage._store[ - "idempotency:change-request:ORD-TEST001:idem-ttl-1" - ]["payload_hash"], + "payload_hash": mock_storage._store[storage_key]["payload_hash"], }, ttl=86400, ) diff --git a/tests/unit/test_change_request_disclosure.py b/tests/unit/test_change_request_disclosure.py new file mode 100644 index 00000000..30d2afe8 --- /dev/null +++ b/tests/unit/test_change_request_disclosure.py @@ -0,0 +1,517 @@ +# Author: Green Mountain Systems AI Inc. +# Donated to IAB Tech Lab + +"""Change request disclosure and actor-spoofing regression tests (seller-928). + +Before the fix, ``GET /api/v1/change-requests`` took OPTIONAL auth, +required no filter, and declared no ``response_model``. One anonymous +request therefore returned every order's change requests, each carrying +``rollback_snapshot`` — a full copy of the order including its +state-machine audit log (every transition's actor and reason), its quote +id, deal id and metadata. + +``POST /api/v1/change-requests`` was likewise anonymous and took +``requested_by`` straight off the wire, so a caller could file a change +request attributed to any actor it cared to name — and the audit trail +that would catch it was the thing being disclosed. + +These tests pin all four halves: the auth gate, the per-actor ownership +scoping, the declared wire shape, and the server-stamped actor. +""" + +import sys +from types import ModuleType +from unittest.mock import AsyncMock, patch + +import pytest + +# Stub execution_activation_flow (cancel-scope leak on ad-server +# connection failure, unresolved -- issue #60 part 2). +_broken_flows = [ + "ad_seller.flows.execution_activation_flow", +] +for _mod_name in _broken_flows: + if _mod_name not in sys.modules: + _stub = ModuleType(_mod_name) + _cls_name = _mod_name.rsplit(".", 1)[-1].replace("_", " ").title().replace(" ", "") + setattr(_stub, _cls_name, type(_cls_name, (), {})) + sys.modules[_mod_name] = _stub + +import httpx # noqa: E402 +from httpx import ASGITransport # noqa: E402 + +from ad_seller.auth.dependencies import actor_from_api_key # noqa: E402 +from ad_seller.interfaces.api.main import app # noqa: E402 +from ad_seller.models.api_key import ( # noqa: E402 + API_KEY_INDEX_PREFIX, + API_KEY_STORAGE_PREFIX, + ApiKeyRecord, + ApiKeyRole, + generate_api_key, + hash_api_key, +) +from ad_seller.models.buyer_identity import BuyerIdentity # noqa: E402 + +# ============================================================================= +# Fixtures +# ============================================================================= + + +@pytest.fixture +def mock_storage(): + store = {} + storage = AsyncMock() + storage.get = AsyncMock(side_effect=lambda k: store.get(k)) + storage.set = AsyncMock(side_effect=lambda k, v, ttl=None: store.__setitem__(k, v)) + storage.keys = AsyncMock( + side_effect=lambda pattern="*": [k for k in store if k.startswith(pattern.rstrip("*"))] + ) + storage.get_order = AsyncMock(side_effect=lambda oid: store.get(f"order:{oid}")) + storage.set_order = AsyncMock( + side_effect=lambda oid, data: store.__setitem__(f"order:{oid}", data) + ) + storage.get_change_request = AsyncMock( + side_effect=lambda cid: store.get(f"change_request:{cid}") + ) + storage.set_change_request = AsyncMock( + side_effect=lambda cid, data: store.__setitem__(f"change_request:{cid}", data) + ) + storage.list_change_requests = AsyncMock( + side_effect=lambda filters=None: [ + v + for k, v in store.items() + if k.startswith("change_request:") + and ( + not filters + or ( + (not filters.get("order_id") or v.get("order_id") == filters["order_id"]) + and (not filters.get("status") or v.get("status") == filters["status"]) + ) + ) + ] + ) + storage._store = store + return storage + + +@pytest.fixture +def client(): + transport = ASGITransport(app=app) + c = httpx.AsyncClient(transport=transport, base_url="http://test") + yield c + app.dependency_overrides.clear() + + +def _seed_key( + store, + *, + role=ApiKeyRole.BUYER, + key_id="key-test", + identity=None, +): + """Seed a valid API key record; return the raw key.""" + raw_key = generate_api_key() + key_hash = hash_api_key(raw_key) + record = ApiKeyRecord( + key_id=key_id, + key_hash=key_hash, + key_prefix_hint=raw_key[:12] + "...", + identity=identity if identity is not None else BuyerIdentity(agency_id="agy-1"), + role=role, + label=f"{role.value} {key_id}", + ) + store[f"{API_KEY_STORAGE_PREFIX}{key_hash}"] = record.model_dump(mode="json") + store[f"{API_KEY_INDEX_PREFIX}{key_id}"] = key_hash + store["api_key_list"] = (store.get("api_key_list") or []) + [key_id] + return raw_key, record + + +def _auth(raw_key: str) -> dict: + return {"Authorization": f"Bearer {raw_key}"} + + +# The order the snapshot would copy: a realistic audit trail with the +# actor/reason pairs and the quote id that must never reach a stranger. +_SECRET_REASON = "renegotiated after Q3 shortfall call with CFO" +_SECRET_QUOTE_ID = "QT-VICTIM-SECRET" + + +def _seed_order(store, order_id="ORD-VICTIM", status="booked"): + store[f"order:{order_id}"] = { + "order_id": order_id, + "status": status, + "deal_id": "DEMO-VICTIM", + "quote_id": _SECRET_QUOTE_ID, + "metadata": {"campaign": "victim-spring-2026"}, + "audit_log": { + "order_id": order_id, + "transitions": [ + { + "from_status": "draft", + "to_status": "booked", + "actor": "human:victim-trader@agency.example", + "reason": _SECRET_REASON, + } + ], + }, + } + return order_id + + +def _create_body(order_id, idem, **extra): + body = { + "idempotency_key": idem, + "order_id": order_id, + "change_type": "creative", + "reason": "swap creative", + } + body.update(extra) + return body + + +# ============================================================================= +# The auth gate +# ============================================================================= + + +class TestChangeRequestsRequireAuth: + async def test_anonymous_list_is_401_and_leaks_nothing(self, client, mock_storage): + """The headline defect: one anonymous unfiltered GET used to return + every order's change requests.""" + order_id = _seed_order(mock_storage._store) + with patch("ad_seller.storage.factory.get_storage", return_value=mock_storage): + victim_key, _ = _seed_key(mock_storage._store, key_id="key-victim") + seeded = await client.post( + "/api/v1/change-requests", + json=_create_body(order_id, "idem-1"), + headers=_auth(victim_key), + ) + assert seeded.status_code == 200 + + resp = await client.get("/api/v1/change-requests") + + assert resp.status_code == 401 + body = resp.text + assert "rollback_snapshot" not in body + assert _SECRET_REASON not in body + assert _SECRET_QUOTE_ID not in body + assert order_id not in body + + async def test_anonymous_read_by_id_is_401(self, client, mock_storage): + order_id = _seed_order(mock_storage._store) + with patch("ad_seller.storage.factory.get_storage", return_value=mock_storage): + victim_key, _ = _seed_key(mock_storage._store, key_id="key-victim") + created = await client.post( + "/api/v1/change-requests", + json=_create_body(order_id, "idem-1"), + headers=_auth(victim_key), + ) + cr_id = created.json()["change_request_id"] + resp = await client.get(f"/api/v1/change-requests/{cr_id}") + + assert resp.status_code == 401 + assert _SECRET_REASON not in resp.text + + async def test_anonymous_create_is_401(self, client, mock_storage): + order_id = _seed_order(mock_storage._store) + with patch("ad_seller.storage.factory.get_storage", return_value=mock_storage): + resp = await client.post( + "/api/v1/change-requests", json=_create_body(order_id, "idem-anon") + ) + assert resp.status_code == 401 + + +# ============================================================================= +# Ownership scoping — authentication alone is not enough +# ============================================================================= + + +class TestChangeRequestsAreScopedToTheCaller: + async def test_another_buyer_list_sees_none_of_the_victims_records(self, client, mock_storage): + """An authenticated stranger must not read the victim's trail.""" + order_id = _seed_order(mock_storage._store) + with patch("ad_seller.storage.factory.get_storage", return_value=mock_storage): + victim_key, _ = _seed_key( + mock_storage._store, + key_id="key-victim", + identity=BuyerIdentity(agency_id="agy-victim"), + ) + attacker_key, _ = _seed_key( + mock_storage._store, + key_id="key-attacker", + identity=BuyerIdentity(agency_id="agy-attacker"), + ) + await client.post( + "/api/v1/change-requests", + json=_create_body(order_id, "idem-1"), + headers=_auth(victim_key), + ) + resp = await client.get("/api/v1/change-requests", headers=_auth(attacker_key)) + + assert resp.status_code == 200 + data = resp.json() + assert data["change_requests"] == [] + assert data["count"] == 0 + assert _SECRET_REASON not in resp.text + assert _SECRET_QUOTE_ID not in resp.text + + async def test_another_buyer_read_by_id_is_404_not_403(self, client, mock_storage): + """404, so the route is not an existence oracle over CR ids.""" + order_id = _seed_order(mock_storage._store) + with patch("ad_seller.storage.factory.get_storage", return_value=mock_storage): + victim_key, _ = _seed_key( + mock_storage._store, + key_id="key-victim", + identity=BuyerIdentity(agency_id="agy-victim"), + ) + attacker_key, _ = _seed_key( + mock_storage._store, + key_id="key-attacker", + identity=BuyerIdentity(agency_id="agy-attacker"), + ) + created = await client.post( + "/api/v1/change-requests", + json=_create_body(order_id, "idem-1"), + headers=_auth(victim_key), + ) + cr_id = created.json()["change_request_id"] + resp = await client.get(f"/api/v1/change-requests/{cr_id}", headers=_auth(attacker_key)) + + assert resp.status_code == 404 + assert _SECRET_REASON not in resp.text + + async def test_owner_still_sees_its_own_records(self, client, mock_storage): + """The gate is scoping, not a blanket denial.""" + order_id = _seed_order(mock_storage._store) + with patch("ad_seller.storage.factory.get_storage", return_value=mock_storage): + victim_key, _ = _seed_key( + mock_storage._store, + key_id="key-victim", + identity=BuyerIdentity(agency_id="agy-victim"), + ) + created = await client.post( + "/api/v1/change-requests", + json=_create_body(order_id, "idem-1"), + headers=_auth(victim_key), + ) + cr_id = created.json()["change_request_id"] + listed = await client.get("/api/v1/change-requests", headers=_auth(victim_key)) + by_id = await client.get(f"/api/v1/change-requests/{cr_id}", headers=_auth(victim_key)) + + assert listed.status_code == 200 + assert listed.json()["count"] == 1 + assert listed.json()["change_requests"][0]["change_request_id"] == cr_id + assert by_id.status_code == 200 + assert by_id.json()["change_request_id"] == cr_id + + async def test_operator_sees_the_whole_queue(self, client, mock_storage): + """Operators review change requests, so their reads stay unscoped.""" + order_id = _seed_order(mock_storage._store) + with patch("ad_seller.storage.factory.get_storage", return_value=mock_storage): + victim_key, _ = _seed_key( + mock_storage._store, + key_id="key-victim", + identity=BuyerIdentity(agency_id="agy-victim"), + ) + op_key, _ = _seed_key(mock_storage._store, role=ApiKeyRole.OPERATOR, key_id="key-op") + await client.post( + "/api/v1/change-requests", + json=_create_body(order_id, "idem-1"), + headers=_auth(victim_key), + ) + resp = await client.get("/api/v1/change-requests", headers=_auth(op_key)) + + assert resp.status_code == 200 + assert resp.json()["count"] == 1 + + async def test_replaying_another_buyers_idempotency_key_does_not_return_its_record( + self, client, mock_storage + ): + """The replay path is a read too — it must not bypass the scoping.""" + order_id = _seed_order(mock_storage._store) + with patch("ad_seller.storage.factory.get_storage", return_value=mock_storage): + victim_key, _ = _seed_key( + mock_storage._store, + key_id="key-victim", + identity=BuyerIdentity(agency_id="agy-victim"), + ) + attacker_key, _ = _seed_key( + mock_storage._store, + key_id="key-attacker", + identity=BuyerIdentity(agency_id="agy-attacker"), + ) + body = _create_body(order_id, "shared-idem-key") + victim = await client.post( + "/api/v1/change-requests", json=body, headers=_auth(victim_key) + ) + attacker = await client.post( + "/api/v1/change-requests", json=body, headers=_auth(attacker_key) + ) + + victim_cr = victim.json()["change_request_id"] + assert attacker.status_code == 200 + assert attacker.json()["change_request_id"] != victim_cr + assert _SECRET_REASON not in attacker.text + + +# ============================================================================= +# The declared wire shape +# ============================================================================= + + +class TestRollbackSnapshotNeverReachesTheWire: + async def test_no_handler_emits_rollback_snapshot(self, client, mock_storage): + """Create, list, read, review and apply are all checked: the snapshot + is still written server-side, but no response model carries it.""" + order_id = _seed_order(mock_storage._store) + with patch("ad_seller.storage.factory.get_storage", return_value=mock_storage): + buyer_key, _ = _seed_key(mock_storage._store, key_id="key-buyer") + op_key, _ = _seed_key(mock_storage._store, role=ApiKeyRole.OPERATOR, key_id="key-op") + + created = await client.post( + "/api/v1/change-requests", + # 'targeting' is MATERIAL, so it lands in pending_approval + # and the review/apply pair is reachable. + json=_create_body(order_id, "idem-1", change_type="targeting"), + headers=_auth(buyer_key), + ) + cr_id = created.json()["change_request_id"] + listed = await client.get("/api/v1/change-requests", headers=_auth(buyer_key)) + read = await client.get(f"/api/v1/change-requests/{cr_id}", headers=_auth(buyer_key)) + reviewed = await client.post( + f"/api/v1/change-requests/{cr_id}/review", + json={"decision": "approve"}, + headers=_auth(op_key), + ) + applied = await client.post( + f"/api/v1/change-requests/{cr_id}/apply", headers=_auth(op_key) + ) + + for label, resp in ( + ("create", created), + ("list", listed), + ("read", read), + ("review", reviewed), + ("apply", applied), + ): + assert resp.status_code == 200, f"{label}: {resp.text}" + assert "rollback_snapshot" not in resp.text, label + assert _SECRET_REASON not in resp.text, label + assert _SECRET_QUOTE_ID not in resp.text, label + assert "audit_log" not in resp.text, label + + # The snapshot is still recorded server-side — this test pins the + # wire, not the storage shape. + stored = mock_storage._store[f"change_request:{cr_id}"] + assert stored["rollback_snapshot"]["order_id"] == order_id + + async def test_every_handler_declares_a_response_model(self): + """No handler may fall back to serialising the service dict.""" + from ad_seller.interfaces.api.routers import change_requests as cr_router + + routes = [r for r in cr_router.router.routes if "change-requests" in r.path] + assert len(routes) == 5 + for route in routes: + assert route.response_model is not None, route.path + + +# ============================================================================= +# The actor is stamped, not asserted +# ============================================================================= + + +class TestRequestedByIsServerStamped: + async def test_body_requested_by_cannot_influence_the_persisted_actor( + self, client, mock_storage + ): + """The social-engineering vector: name yourself the publisher's CFO + so the human reviewer approves.""" + order_id = _seed_order(mock_storage._store) + with patch("ad_seller.storage.factory.get_storage", return_value=mock_storage): + buyer_key, record = _seed_key( + mock_storage._store, + key_id="key-buyer", + identity=BuyerIdentity(agency_id="agy-1"), + ) + resp = await client.post( + "/api/v1/change-requests", + json=_create_body(order_id, "idem-1", requested_by="human:cfo@publisher.example"), + headers=_auth(buyer_key), + ) + + assert resp.status_code == 200 + assert resp.json()["requested_by"] == actor_from_api_key(record) + assert "cfo@publisher.example" not in resp.text + cr_id = resp.json()["change_request_id"] + stored = mock_storage._store[f"change_request:{cr_id}"] + assert stored["requested_by"] == actor_from_api_key(record) + + def test_create_model_has_no_requested_by_field(self): + from ad_seller.interfaces.api.schemas import CreateChangeRequestModel + + assert "requested_by" not in CreateChangeRequestModel.model_fields + + def test_actor_is_stable_across_key_rotation(self): + """The actor is an ownership key: rotating a credential must not + orphan the records filed under the old one.""" + identity = BuyerIdentity(agency_id="agy-1") + old = ApiKeyRecord(key_id="key-old", key_hash="h1", key_prefix_hint="p", identity=identity) + new = ApiKeyRecord(key_id="key-new", key_hash="h2", key_prefix_hint="p", identity=identity) + assert actor_from_api_key(old) == actor_from_api_key(new) == "agent:agy-1" + + def test_operator_actor_cannot_collide_with_a_buyer_identifier(self): + operator = ApiKeyRecord( + key_id="agy-1", + key_hash="h", + key_prefix_hint="p", + identity=BuyerIdentity(), + role=ApiKeyRole.OPERATOR, + ) + buyer = ApiKeyRecord( + key_id="key-b", + key_hash="h", + key_prefix_hint="p", + identity=BuyerIdentity(agency_id="agy-1"), + ) + assert actor_from_api_key(operator) != actor_from_api_key(buyer) + + def test_identityless_buyer_key_falls_back_to_its_key_id(self): + """No stable buyer identity to anchor to — must not collapse every + such key onto one shared 'public' actor.""" + a = ApiKeyRecord( + key_id="key-a", key_hash="h", key_prefix_hint="p", identity=BuyerIdentity() + ) + b = ApiKeyRecord( + key_id="key-b", key_hash="h", key_prefix_hint="p", identity=BuyerIdentity() + ) + assert actor_from_api_key(a) != actor_from_api_key(b) + + +# ============================================================================= +# The service-level scoping, so no future consumer can re-open it +# ============================================================================= + + +class TestServiceLevelScoping: + async def test_list_scope_is_not_pushed_into_the_storage_filter(self, mock_storage): + """StorageBackend.list_change_requests ignores unknown filter keys — + a scope passed that way would fail OPEN.""" + from ad_seller.services import order_service + + mock_storage._store["change_request:CR-1"] = { + "change_request_id": "CR-1", + "order_id": "ORD-A", + "requested_by": "agent:agy-victim", + } + mock_storage._store["change_request:CR-2"] = { + "change_request_id": "CR-2", + "order_id": "ORD-A", + "requested_by": "agent:agy-attacker", + } + with patch("ad_seller.storage.factory.get_storage", return_value=mock_storage): + result = await order_service.list_change_requests(requested_by="agent:agy-attacker") + + assert result["count"] == 1 + assert result["change_requests"][0]["change_request_id"] == "CR-2" + passed_filters = mock_storage.list_change_requests.await_args.args[0] + assert passed_filters is None or "requested_by" not in passed_filters diff --git a/tests/unit/test_operator_auth.py b/tests/unit/test_operator_auth.py index d7914d88..3e1fadb3 100644 --- a/tests/unit/test_operator_auth.py +++ b/tests/unit/test_operator_auth.py @@ -21,7 +21,11 @@ (f) The remaining mutation surface is gated: inventory-type override write/delete, change-request review/apply, and order transitions (REST endpoint and its MCP twin) — while the corresponding - buyer-facing reads and CR submission stay on the optional-key path. + buyer-facing reads stay on the optional-key path. Change-request + submission and reads are the exception: they are buyer-facing (a + BUYER key suffices) but no longer anonymous, because the records + embed the order's audit trail (seller-928 — scoping covered in + test_change_request_disclosure.py). """ import sys @@ -893,18 +897,25 @@ async def test_operator_key_apply_passes_auth(self, client, mock_storage): assert resp.status_code == 200 assert resp.json()["status"] == "applied" - async def test_create_and_read_change_requests_stay_buyer_facing(self, client, mock_storage): - """POST /change-requests and the reads remain on the optional-key - surface — anonymous callers are not rejected by auth.""" + async def test_create_and_read_change_requests_are_buyer_facing_but_credentialed( + self, client, mock_storage + ): + """Submission and the reads stay buyer-facing — a BUYER key is + enough, no operator credential needed — but they are no longer + anonymous: the listing used to return every order's audit trail to + anyone who asked (seller-928).""" + buyer_key = _seed_key(mock_storage._store, role=ApiKeyRole.BUYER) with ( patch("ad_seller.storage.factory.get_storage", return_value=mock_storage), patch( "ad_seller.services.order_service.list_change_requests", - new=AsyncMock(return_value=[]), + new=AsyncMock(return_value={"change_requests": [], "count": 0}), ), ): - resp = await client.get("/api/v1/change-requests") - assert resp.status_code == 200 + anonymous = await client.get("/api/v1/change-requests") + credentialed = await client.get("/api/v1/change-requests", headers=_auth(buyer_key)) + assert anonymous.status_code == 401 + assert credentialed.status_code == 200 class TestOrderTransitionRequiresOperator: From f6bd39bdfbb6527145f11e1d21c4469fc877401a Mon Sep 17 00:00:00 2001 From: ATC964 <82515918+atc964@users.noreply.github.com> Date: Tue, 22 Sep 2026 13:31:02 -0400 Subject: [PATCH 2/2] docs: regenerate openapi.json for the new response models --- docs/api/openapi.json | 326 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 312 insertions(+), 14 deletions(-) diff --git a/docs/api/openapi.json b/docs/api/openapi.json index 4711c219..ad2f75a7 100644 --- a/docs/api/openapi.json +++ b/docs/api/openapi.json @@ -1937,6 +1937,86 @@ } } } + }, + "get": { + "tags": [ + "Deal Booking" + ], + "summary": "List Deals", + "description": "List stored deals, optionally filtered by wire status.\n\nOperator-only: the list spans every buyer's deals. Buyers read their\nown deal with ``GET /api/v1/deals/{deal_id}``.\n\n``status`` is the shared :class:`DealStatus` value the deal carries on\nthe wire (the same value ``GET /api/v1/deals/{deal_id}`` returns), so\nthe filter is applied after mapping each stored record to its\nresponse shape.\n\nRegistered ahead of ``/api/v1/deals/{deal_id}``; the two paths differ in\nlength so there is no shadowing, but keeping literal routes together\nwith ``/export`` keeps the EP-8.4 ordering rule easy to audit.", + "operationId": "list_deals_api_v1_deals_get", + "parameters": [ + { + "name": "status", + "in": "query", + "required": false, + "schema": { + "anyOf": [ + { + "$ref": "#/components/schemas/DealStatus" + }, + { + "type": "null" + } + ], + "title": "Status" + } + }, + { + "name": "authorization", + "in": "header", + "required": false, + "schema": { + "anyOf": [ + { + "type": "string" + }, + { + "type": "null" + } + ], + "title": "Authorization" + } + }, + { + "name": "X-Api-Key", + "in": "header", + "required": false, + "schema": { + "anyOf": [ + { + "type": "string" + }, + { + "type": "null" + } + ], + "title": "X-Api-Key" + } + } + ], + "responses": { + "200": { + "description": "Successful Response", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/DealListResponse" + } + } + } + }, + "422": { + "description": "Validation Error", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/HTTPValidationError" + } + } + } + } + } } }, "/agentic-audience/match": { @@ -5398,7 +5478,7 @@ "Change Requests" ], "summary": "Create Change Request", - "description": "Submit a change request for an existing order.\n\nValidates the change against the current order state, classifies\nseverity, and routes to approval if needed.\n\n**Idempotency (FD-12):** the request carries a required\n``idempotency_key``. An identical replay returns the original change\nrequest without creating a second approval; reusing the key with a\ndifferent body returns ``idempotency_conflict`` (HTTP 409). Keys are\nscoped per order and expire after 24 hours.", + "description": "Submit a change request for an existing order. Requires authentication.\n\nThe requesting actor is stamped from the presented credential, never\nfrom the body \u2014 ``requested_by`` is exactly the field a human\nreviewer trusts when deciding whether to approve, so a caller must\nnot be able to name itself \"human:cfo@publisher.example\".\n\n**Idempotency (FD-12):** the request carries a required\n``idempotency_key``. An identical replay returns the original change\nrequest without creating a second approval; reusing the key with a\ndifferent body returns ``idempotency_conflict`` (HTTP 409). Keys are\nscoped per actor and order, and expire after 24 hours.", "operationId": "create_change_request_api_v1_change_requests_post", "parameters": [ { @@ -5449,7 +5529,9 @@ "description": "Successful Response", "content": { "application/json": { - "schema": {} + "schema": { + "$ref": "#/components/schemas/ChangeRequestResponse" + } } } }, @@ -5470,7 +5552,7 @@ "Change Requests" ], "summary": "List Change Requests", - "description": "List change requests, optionally filtered by order or status.", + "description": "List change requests, optionally filtered by order or status.\n\nRequires authentication and returns only the caller's own change\nrequests; an operator credential sees all of them.", "operationId": "list_change_requests_api_v1_change_requests_get", "parameters": [ { @@ -5543,7 +5625,9 @@ "description": "Successful Response", "content": { "application/json": { - "schema": {} + "schema": { + "$ref": "#/components/schemas/ChangeRequestListResponse" + } } } }, @@ -5566,7 +5650,7 @@ "Change Requests" ], "summary": "Get Change Request", - "description": "Get a change request by ID.", + "description": "Get a change request by ID.\n\nRequires authentication. Another actor's change request reads as 404,\nso the route cannot be walked as an existence oracle over change\nrequest ids.", "operationId": "get_change_request_api_v1_change_requests__cr_id__get", "parameters": [ { @@ -5616,7 +5700,9 @@ "description": "Successful Response", "content": { "application/json": { - "schema": {} + "schema": { + "$ref": "#/components/schemas/ChangeRequestResponse" + } } } }, @@ -5699,7 +5785,9 @@ "description": "Successful Response", "content": { "application/json": { - "schema": {} + "schema": { + "$ref": "#/components/schemas/ChangeRequestResponse" + } } } }, @@ -5772,7 +5860,9 @@ "description": "Successful Response", "content": { "application/json": { - "schema": {} + "schema": { + "$ref": "#/components/schemas/ChangeRequestApplyResponse" + } } } }, @@ -6557,6 +6647,189 @@ "title": "CancellationTerms", "description": "Structured cancellation window for linear TV deals." }, + "ChangeRequestApplyResponse": { + "properties": { + "change_request_id": { + "type": "string", + "title": "Change Request Id", + "default": "" + }, + "status": { + "type": "string", + "title": "Status", + "default": "" + }, + "order_id": { + "type": "string", + "title": "Order Id", + "default": "" + } + }, + "type": "object", + "title": "ChangeRequestApplyResponse", + "description": "Wire shape of ``POST /api/v1/change-requests/{cr_id}/apply``." + }, + "ChangeRequestListResponse": { + "properties": { + "change_requests": { + "items": { + "$ref": "#/components/schemas/ChangeRequestResponse" + }, + "type": "array", + "title": "Change Requests", + "default": [] + }, + "count": { + "type": "integer", + "title": "Count", + "default": 0 + } + }, + "type": "object", + "title": "ChangeRequestListResponse", + "description": "Wire shape of ``GET /api/v1/change-requests``." + }, + "ChangeRequestResponse": { + "properties": { + "change_request_id": { + "type": "string", + "title": "Change Request Id", + "default": "" + }, + "order_id": { + "type": "string", + "title": "Order Id", + "default": "" + }, + "deal_id": { + "type": "string", + "title": "Deal Id", + "default": "" + }, + "status": { + "type": "string", + "title": "Status", + "default": "" + }, + "change_type": { + "type": "string", + "title": "Change Type", + "default": "" + }, + "severity": { + "type": "string", + "title": "Severity", + "default": "" + }, + "requested_by": { + "type": "string", + "title": "Requested By", + "default": "" + }, + "requested_at": { + "anyOf": [ + { + "type": "string" + }, + { + "type": "null" + } + ], + "title": "Requested At" + }, + "reason": { + "type": "string", + "title": "Reason", + "default": "" + }, + "diffs": { + "items": { + "$ref": "#/components/schemas/FieldDiffModel" + }, + "type": "array", + "title": "Diffs", + "default": [] + }, + "proposed_values": { + "additionalProperties": true, + "type": "object", + "title": "Proposed Values", + "default": {} + }, + "validation_errors": { + "items": { + "type": "string" + }, + "type": "array", + "title": "Validation Errors", + "default": [] + }, + "pricing_impact": { + "anyOf": [ + { + "additionalProperties": true, + "type": "object" + }, + { + "type": "null" + } + ], + "title": "Pricing Impact" + }, + "availability_check": { + "anyOf": [ + { + "additionalProperties": true, + "type": "object" + }, + { + "type": "null" + } + ], + "title": "Availability Check" + }, + "approved_by": { + "type": "string", + "title": "Approved By", + "default": "" + }, + "approved_at": { + "anyOf": [ + { + "type": "string" + }, + { + "type": "null" + } + ], + "title": "Approved At" + }, + "rejection_reason": { + "type": "string", + "title": "Rejection Reason", + "default": "" + }, + "applied_at": { + "anyOf": [ + { + "type": "string" + }, + { + "type": "null" + } + ], + "title": "Applied At" + }, + "applied_by": { + "type": "string", + "title": "Applied By", + "default": "" + } + }, + "type": "object", + "title": "ChangeRequestResponse", + "description": "Wire shape of a change request.\n\nDeclared explicitly so the router cannot serialise the stored record\nwholesale. In particular ``rollback_snapshot`` \u2014 a full copy of the\norder, including its state-machine audit log, every transition's\nactor and reason, and its quote/deal ids \u2014 is a SERVER-SIDE field and\nis absent here by construction, not by filtering." + }, "CommercialTerms": { "properties": { "supported_deal_types": { @@ -6902,11 +7175,6 @@ "type": "string", "title": "Reason", "default": "" - }, - "requested_by": { - "type": "string", - "title": "Requested By", - "default": "system" } }, "type": "object", @@ -6916,7 +7184,7 @@ "change_type" ], "title": "CreateChangeRequestModel", - "description": "Request to create a change request for an order." + "description": "Request to create a change request for an order.\n\n``requested_by`` is deliberately ABSENT. The actor is stamped\nserver-side from the presented credential\n(``auth.dependencies.actor_from_api_key``) \u2014 it is the field a human\nreviewer trusts when approving, so it must never be a wire string the\nrequester chooses. A body that still carries ``requested_by`` is\naccepted and the value ignored (pydantic drops unknown fields)." }, "CreateOperatorApiKeyRequest": { "properties": { @@ -7700,6 +7968,36 @@ "title": "DealFromTemplateResponse", "description": "Response for template-based deal creation." }, + "DealListResponse": { + "properties": { + "deals": { + "items": { + "$ref": "#/components/schemas/DealBookingResponse" + }, + "type": "array", + "title": "Deals" + }, + "count": { + "type": "integer", + "title": "Count" + }, + "skipped": { + "items": { + "type": "string" + }, + "type": "array", + "title": "Skipped", + "default": [] + } + }, + "type": "object", + "required": [ + "deals", + "count" + ], + "title": "DealListResponse", + "description": "Page of stored deals in the shared booking-response shape." + }, "DealMigrationRequest": { "properties": { "old_deal_id": {