diff --git a/examples/v3_reference_seller/alembic/versions/0003_unique_buyer_api_key.py b/examples/v3_reference_seller/alembic/versions/0003_unique_buyer_api_key.py new file mode 100644 index 000000000..a02a161dd --- /dev/null +++ b/examples/v3_reference_seller/alembic/versions/0003_unique_buyer_api_key.py @@ -0,0 +1,69 @@ +"""require globally unique buyer-agent bearer identifiers + +Revision ID: 0003 +Revises: 0002 +Create Date: 2026-07-29 + +``api_key_id`` is a bearer credential, so it must identify exactly one +commercial identity and tenant. The preflight deliberately stops the +migration before changing indexes when legacy duplicates exist; operators +must rotate or remove duplicates rather than letting the database choose an +arbitrary owner. +""" + +from __future__ import annotations + +from collections.abc import Sequence + +import sqlalchemy as sa +from alembic import context, op + +revision: str = "0003" +down_revision: str | None = "0002" +branch_labels: str | Sequence[str] | None = None +depends_on: str | Sequence[str] | None = None + + +def upgrade() -> None: + if not context.is_offline_mode(): + connection = op.get_bind() + duplicate_count = connection.execute( + sa.text( + "SELECT COUNT(*) FROM (" + "SELECT api_key_id FROM buyer_agents " + "WHERE api_key_id IS NOT NULL " + "GROUP BY api_key_id HAVING COUNT(*) > 1" + ") AS duplicate_credentials" + ) + ).scalar_one() + if duplicate_count: + raise RuntimeError( + "Cannot enforce buyer_agents.api_key_id uniqueness: " + f"found {duplicate_count} duplicated credential identifier(s). " + "Rotate or remove duplicate bearer credentials, then rerun the migration." + ) + + # Create first so a concurrent/legacy duplicate makes the migration fail + # while the old lookup index remains available. PostgreSQL then rolls the + # transaction back; offline SQL retains the same safe ordering. + op.create_index( + "buyer_agents_api_key_uidx", + "buyer_agents", + ["api_key_id"], + unique=True, + postgresql_where=sa.text("api_key_id IS NOT NULL"), + sqlite_where=sa.text("api_key_id IS NOT NULL"), + ) + op.drop_index("buyer_agents_api_key_idx", table_name="buyer_agents") + + +def downgrade() -> None: + op.drop_index("buyer_agents_api_key_uidx", table_name="buyer_agents") + op.create_index( + "buyer_agents_api_key_idx", + "buyer_agents", + ["api_key_id"], + unique=False, + postgresql_where=sa.text("api_key_id IS NOT NULL"), + sqlite_where=sa.text("api_key_id IS NOT NULL"), + ) diff --git a/examples/v3_reference_seller/src/app.py b/examples/v3_reference_seller/src/app.py index 4b105119a..a2c7c791e 100644 --- a/examples/v3_reference_seller/src/app.py +++ b/examples/v3_reference_seller/src/app.py @@ -62,9 +62,14 @@ from adcp.server import ( SubdomainTenantMiddleware, ToolContext, - current_tenant, ) -from adcp.server.auth import BearerTokenAuth, Principal, auth_context_factory +from adcp.server.auth import ( + ROUTED_TENANT_METADATA_KEY, + BearerTokenAuth, + Principal, + auth_context_factory, + enforce_authenticated_tenant, +) from adcp.validation import ValidationHookConfig from adcp.webhook_sender import WebhookSender from adcp.webhook_supervisor import InMemoryWebhookDeliverySupervisor @@ -104,12 +109,12 @@ def _build_context_factory(): def build(meta: RequestMetadata) -> ToolContext: ctx = auth_context_factory(meta) - # Pin tenant from SubdomainTenantMiddleware. Subdomain wins for - # tenant routing; the validator's tenant_id is only the token's - # home tenant and may not match the host the request came in on. - tenant = current_tenant() - if tenant is not None: - ctx = replace(ctx, tenant_id=tenant.id) + # ``auth_context_factory`` preserves both identities in metadata. + # Pin the business context to the routed host; the skill middleware + # below compares it with the authenticated identity from the token at + # a boundary where both MCP and A2A project AdcpError consistently. + if ROUTED_TENANT_METADATA_KEY in ctx.metadata: + ctx = replace(ctx, tenant_id=ctx.metadata[ROUTED_TENANT_METADATA_KEY]) # Upgrade bearer-flow auth_info with a typed ApiKeyCredential # when the validator stashed the raw token in principal metadata. @@ -151,6 +156,11 @@ async def _load_token_map(sessionmaker) -> dict[str, Principal]: select(BuyerAgentRow).where(BuyerAgentRow.api_key_id.is_not(None)) ) for row in result.scalars(): + if row.api_key_id in token_map: + raise RuntimeError( + "Duplicate buyer-agent api_key_id detected; bearer credentials " + "must identify exactly one tenant." + ) token_map[row.api_key_id] = Principal( caller_identity=row.agent_url, tenant_id=row.tenant_id, @@ -366,6 +376,7 @@ def main() -> None: # registry with credential=None and returns PERMISSION_DENIED. auth=BearerTokenAuth(validate_token=_make_validate_token(token_map)), context_factory=_build_context_factory(), + middleware=[enforce_authenticated_tenant], asgi_middleware=[ (SubdomainTenantMiddleware, {"router": router}), ], diff --git a/examples/v3_reference_seller/src/buyer_registry.py b/examples/v3_reference_seller/src/buyer_registry.py index 978a69ad7..fabd532f9 100644 --- a/examples/v3_reference_seller/src/buyer_registry.py +++ b/examples/v3_reference_seller/src/buyer_registry.py @@ -100,13 +100,23 @@ async def resolve_by_credential( key = credential.client_id async with self._sessionmaker() as session: result = await session.execute( - select(BuyerAgentRow).where( + select(BuyerAgentRow) + .where( BuyerAgentRow.tenant_id == tenant.id, BuyerAgentRow.api_key_id == key, ) + .limit(2) ) - row = result.scalar_one_or_none() - return _row_to_agent(row) if row else None + rows = list(result.scalars().all()) + if len(rows) > 1: + logger.error( + "ambiguous buyer credential within tenant; denying lookup " + "(tenant_id=%s, credential_kind=%s)", + tenant.id, + credential.kind, + ) + return None + return _row_to_agent(rows[0]) if rows else None def _row_to_agent(row: BuyerAgentRow) -> BuyerAgent: diff --git a/examples/v3_reference_seller/src/models.py b/examples/v3_reference_seller/src/models.py index 119e49c56..252641e71 100644 --- a/examples/v3_reference_seller/src/models.py +++ b/examples/v3_reference_seller/src/models.py @@ -194,9 +194,11 @@ class BuyerAgent(Base): UniqueConstraint("tenant_id", "agent_url", name="buyer_agents_tenant_agent_uk"), Index("buyer_agents_tenant_idx", "tenant_id"), Index( - "buyer_agents_api_key_idx", + "buyer_agents_api_key_uidx", "api_key_id", - postgresql_where=(api_key_id.is_not(None)), # type: ignore[has-type] + unique=True, + postgresql_where=(api_key_id.is_not(None)), + sqlite_where=(api_key_id.is_not(None)), ), ) diff --git a/examples/v3_reference_seller/src/platform.py b/examples/v3_reference_seller/src/platform.py index d0635d5c5..56724f9e9 100644 --- a/examples/v3_reference_seller/src/platform.py +++ b/examples/v3_reference_seller/src/platform.py @@ -77,9 +77,11 @@ AdcpError, DecisioningCapabilities, DecisioningPlatform, + MediaBuyNotFoundError, MockAdServer, RefinementOutcome, RefineResult, + ServiceUnavailableError, StaticBearer, SyncAccountsResultRow, UpstreamHttpClient, @@ -318,11 +320,38 @@ async def resolve( if ref is not None: account_id = ref.get("account_id") + principal: str | None = None + if auth_info is not None: + principal = getattr(auth_info, "principal", None) + if not principal: + raise AdcpError( + "AUTH_MISSING", + message="Account resolution requires an authenticated buyer-agent principal.", + recovery="correctable", + ) + async with sessionmaker() as session: + ba_result = await session.execute( + select(BuyerAgentRow).where( + BuyerAgentRow.tenant_id == tenant.id, + BuyerAgentRow.agent_url == principal, + ) + ) + buyer_agent = ba_result.scalar_one_or_none() + if buyer_agent is None: + raise AdcpError( + "ACCOUNT_NOT_FOUND", + message=( + f"No buyer agent matches principal {principal!r} " + f"under tenant {tenant.id!r}." + ), + recovery="terminal", + ) if account_id: result = await session.execute( select(AccountRow).where( AccountRow.tenant_id == tenant.id, + AccountRow.buyer_agent_id == buyer_agent.id, AccountRow.account_id == account_id, AccountRow.status == "active", ) @@ -332,7 +361,7 @@ async def resolve( raise AdcpError( "ACCOUNT_NOT_FOUND", message=( - f"No active account {account_id!r} under " f"tenant {tenant.id!r}." + f"No active account {account_id!r} under tenant {tenant.id!r}." ), recovery="terminal", field="account.account_id", @@ -344,39 +373,6 @@ async def resolve( # buyer agent. The buyer-agent's agent_url is the # `principal` field on auth_info — populated by the # framework's auth middleware from the validated bearer. - principal: str | None = None - if auth_info is not None: - principal = getattr(auth_info, "principal", None) - if not principal: - raise AdcpError( - "ACCOUNT_NOT_FOUND", - message=( - "Request did not include `account.account_id` " - "and no authenticated buyer-agent principal was " - "available to resolve a brand-shaped reference. " - "Send `account.account_id` explicitly, or " - "authenticate with a bearer token bound to a " - "seeded buyer agent." - ), - recovery="correctable", - field="account.account_id", - ) - ba_result = await session.execute( - select(BuyerAgentRow).where( - BuyerAgentRow.tenant_id == tenant.id, - BuyerAgentRow.agent_url == principal, - ) - ) - buyer_agent = ba_result.scalar_one_or_none() - if buyer_agent is None: - raise AdcpError( - "ACCOUNT_NOT_FOUND", - message=( - f"No buyer agent matches principal {principal!r} " - f"under tenant {tenant.id!r}." - ), - recovery="terminal", - ) acct_result = await session.execute( select(AccountRow) .where( @@ -444,7 +440,7 @@ async def upsert( ), recovery="terminal", ) - for incoming in refs: + for index, incoming in enumerate(refs): brand_domain = incoming.brand.domain natural_account_id = f"{brand_domain}::{incoming.operator}" billing_entity_payload: dict[str, Any] | None = None @@ -478,6 +474,28 @@ async def upsert( session.add(new_row) action: str = "created" else: + if existing.buyer_agent_id != buyer_agent_row.id: + rows.append( + SyncAccountsResultRow( + brand=incoming.brand.model_dump(mode="json", exclude_none=True), + operator=incoming.operator, + action="failed", + status="rejected", + errors=[ + { + "code": "ACCOUNT_NOT_FOUND", + "message": ( + "Account is not visible to the authenticated " + "buyer agent." + ), + "recovery": "terminal", + "field": f"accounts[{index}]", + } + ], + sandbox=bool(incoming.sandbox), + ) + ) + continue existing.billing = billing_value existing.billing_entity = billing_entity_payload existing.sandbox = bool(incoming.sandbox) @@ -853,19 +871,21 @@ def __init__( # Seller-local shadow store for state the mock-server doesn't # model: per-package ``targeting_overlay`` / ``measurement_terms`` # echo data, plus media-buy and per-package ``canceled`` / ``paused`` - # flags. Keyed by upstream ``order_id``. Real adopters whose ad - # server tracks this shape upstream drop the shadow store. - self._buy_state: dict[str, dict[str, Any]] = {} + # flags. Keyed by ``(account_scope, upstream order_id)`` so two + # buyer accounts mapped to the same upstream advertiser cannot see + # or mutate one another's local state. Real adopters whose ad server + # tracks this shape upstream drop the shadow store. + self._buy_state: dict[tuple[str, str], dict[str, Any]] = {} # Monotonic per-buy revision counter (update_media_buy's # optimistic-concurrency token). - self._buy_revisions: dict[str, int] = {} + self._buy_revisions: dict[tuple[str, str], int] = {} # Bidirectional buyer-creative-id ↔ upstream-creative-id map. # The upstream mints ``cr_`` on every upload regardless of # ``client_request_id``, so the seller has to track the mapping # to (a) echo the buyer's id in ``list_creatives`` and (b) # translate before calling ``attach_creative`` upstream. - self._creative_id_map: dict[str, str] = {} # buyer_id → upstream_id - self._creative_id_reverse: dict[str, str] = {} # upstream_id → buyer_id + self._creative_id_map: dict[tuple[str, str], str] = {} + self._creative_id_reverse: dict[tuple[str, str], str] = {} # AccountStore is always wired. ``app.main`` passes the # MOCK_AD_SERVER_URL env so resolved accounts route at the JS # mock-server fixture. Tests that bypass the AccountStore (by @@ -899,6 +919,63 @@ def _client(self, ctx: RequestContext) -> UpstreamHttpClient: treat_404_as_none=False, ) + @staticmethod + def _account_scope(ctx: RequestContext) -> str: + """Return the stable account boundary for seller-local state.""" + if ctx.account is None: + raise AdcpError( + "AUTH_REQUIRED", + message="Account-scoped seller state requires an authenticated account.", + recovery="terminal", + ) + tenant_id = str(ctx.account.metadata.get("tenant_id") or "") + if not tenant_id: + raise AdcpError( + "AUTH_REQUIRED", + message="Account-scoped seller state requires an authenticated tenant.", + recovery="terminal", + ) + return f"{tenant_id}:{ctx.account.id}" + + def _buy_key(self, ctx: RequestContext, order_id: str) -> tuple[str, str]: + """Return the composite owner key for a seller-local media buy.""" + return (self._account_scope(ctx), order_id) + + async def _get_owned_order( + self, + ctx: RequestContext, + client: UpstreamHttpClient, + *, + network_code: str, + order_id: str, + ) -> dict[str, Any]: + """Fetch an order and fail closed unless it belongs to the account.""" + not_found = MediaBuyNotFoundError( + media_buy_id=order_id, + message=f"Media buy {order_id!r} was not found.", + field="media_buy_id", + ) + try: + order = await upstream_helpers.get_order( + client, network_code=network_code, order_id=order_id + ) + except AdcpError as exc: + if exc.code == "MEDIA_BUY_NOT_FOUND": + raise not_found from exc + raise + if "advertiser_id" not in order: + raise ServiceUnavailableError( + message="Upstream order response omitted required ownership metadata." + ) + expected_advertiser = ctx.account.metadata["advertiser_id"] + if ( + order.get("advertiser_id") != expected_advertiser + or self._buy_key(ctx, order_id) not in self._buy_state + ): + # Keep foreign and nonexistent ids indistinguishable. + raise not_found + return order + def _record(self, method: str, args: dict[str, Any]) -> None: """Record an outbound upstream call on the wired :class:`MockAdServer`, if any. @@ -1105,12 +1182,14 @@ async def create_media_buy(self, req: CreateMediaBuyRequest, ctx: RequestContext ) order_id: str = order["order_id"] + buy_key = self._buy_key(ctx, order_id) + self._buy_state.setdefault(buy_key, {"packages": {}, "canceled": False}) approval_task_id: str | None = order.get("approval_task_id") # Sync fast path — the upstream may auto-approve on creation # for non-guaranteed delivery (rare, but possible). if order.get("status") in {"approved", "delivering"} and not approval_task_id: return await self._project_create_success( - order, req, budget_amount, budget_currency, client, network_code + order, req, budget_amount, budget_currency, client, network_code, ctx ) # No approval task but status not already terminal-success — @@ -1126,7 +1205,7 @@ async def create_media_buy(self, req: CreateMediaBuyRequest, ctx: RequestContext {"order_id": order_id, "status": current.get("status")}, ) return await self._finalize_create_or_raise( - current, req, budget_amount, budget_currency, client, network_code + current, req, budget_amount, budget_currency, client, network_code, ctx ) # Slow path — hand off to background polling. The framework @@ -1196,7 +1275,7 @@ async def _poll_until_approved(task_handoff_ctx: Any) -> CreateMediaBuySuccessRe {"order_id": order_id, "status": approved_order.get("status")}, ) return await self._finalize_create_or_raise( - approved_order, req, budget_amount, budget_currency, client, network_code + approved_order, req, budget_amount, budget_currency, client, network_code, ctx ) # Reference seller is mock-mode against a fast upstream — auto-approval @@ -1254,6 +1333,7 @@ async def _finalize_create_or_raise( budget_currency: str, client: UpstreamHttpClient, network_code: str, + ctx: RequestContext, ) -> CreateMediaBuySuccessResponse: """Project a terminal upstream order onto a buyer-facing success response — but refuse to fabricate success when the upstream is @@ -1284,7 +1364,7 @@ async def _finalize_create_or_raise( recovery="transient", ) return await self._project_create_success( - order, req, budget_amount, budget_currency, client, network_code + order, req, budget_amount, budget_currency, client, network_code, ctx ) async def _project_create_success( @@ -1295,6 +1375,7 @@ async def _project_create_success( budget_currency: str, client: UpstreamHttpClient, network_code: str, + ctx: RequestContext, ) -> CreateMediaBuySuccessResponse: """Translate upstream ``Order`` to AdCP :class:`CreateMediaBuySuccessResponse`. @@ -1323,10 +1404,11 @@ async def _project_create_success( if no_creatives_supplied: wire_status = "pending_creatives" order_id = order["order_id"] - buy_state = self._buy_state.setdefault(order_id, {"packages": {}, "canceled": False}) + buy_key = self._buy_key(ctx, order_id) + buy_state = self._buy_state.setdefault(buy_key, {"packages": {}, "canceled": False}) if req.context is not None: buy_state["context"] = req.context.model_dump(mode="json", exclude_none=True) - revision = self._buy_revisions.setdefault(order_id, 1) + revision = self._buy_revisions.setdefault(buy_key, 1) response_packages: list[dict[str, Any]] = [] for idx, pkg in enumerate(req_packages): line_item = await upstream_helpers.add_line_item( @@ -1389,7 +1471,7 @@ async def update_media_buy( # Validate the media buy exists upstream. The SDK maps a 404 onto # ``MEDIA_BUY_NOT_FOUND`` automatically. - await upstream_helpers.get_order(client, network_code=network_code, order_id=media_buy_id) + await self._get_owned_order(ctx, client, network_code=network_code, order_id=media_buy_id) # Validate referenced packages exist on the order. The mock's # ``serializeOrder`` strips ``line_items`` from ``GET /orders/{id}`` @@ -1414,8 +1496,9 @@ async def update_media_buy( recovery="terminal", ) + buy_key = self._buy_key(ctx, media_buy_id) buy_state = self._buy_state.setdefault( - media_buy_id, {"packages": {}, "canceled": False, "paused": False} + buy_key, {"packages": {}, "canceled": False, "paused": False} ) # Buy-level cancel — irreversible. A second cancel is NOT_CANCELLABLE. @@ -1472,7 +1555,8 @@ async def update_media_buy( # before issuing attach_creative. Pass through unchanged # when no mapping is known (the upstream will surface # a 404 → CREATIVE_NOT_FOUND). - upstream_creative_id = self._creative_id_map.get(creative_id, creative_id) + creative_key = (self._account_scope(ctx), creative_id) + upstream_creative_id = self._creative_id_map.get(creative_key, creative_id) await upstream_helpers.attach_creative( client, network_code=network_code, @@ -1490,8 +1574,8 @@ async def update_media_buy( affected_packages.append({"package_id": pkg_id, **_projected_package_state(pkg_state)}) # Bump the optimistic-concurrency revision token. - revision = self._buy_revisions.get(media_buy_id, 0) + 1 - self._buy_revisions[media_buy_id] = revision + revision = self._buy_revisions.get(buy_key, 0) + 1 + self._buy_revisions[buy_key] = revision # Compute response status. Cancel beats pause beats whatever the # upstream says — buyer's intent is the source of truth for the @@ -1555,7 +1639,8 @@ async def sync_creatives( # the seller treats the second call as "creative already # known, just acknowledge". The buyer's intent for the new # placement flows through the ``assignments`` field below. - if creative.creative_id in self._creative_id_map: + creative_key = (self._account_scope(ctx), creative.creative_id) + if creative_key in self._creative_id_map: results.append( SyncCreativeResult.model_validate( { @@ -1589,8 +1674,10 @@ async def sync_creatives( ) upstream_id = str(upstream_resp.get("creative_id") or "") if upstream_id: - self._creative_id_map[creative.creative_id] = upstream_id - self._creative_id_reverse[upstream_id] = creative.creative_id + self._creative_id_map[creative_key] = upstream_id + self._creative_id_reverse[(self._account_scope(ctx), upstream_id)] = ( + creative.creative_id + ) results.append( SyncCreativeResult.model_validate( { @@ -1609,19 +1696,22 @@ async def sync_creatives( package_id = getattr(assignment, "package_id", None) if not buyer_creative_id or not package_id: continue - upstream_creative_id = self._creative_id_map.get(buyer_creative_id, buyer_creative_id) + creative_key = (self._account_scope(ctx), buyer_creative_id) + upstream_creative_id = self._creative_id_map.get(creative_key, buyer_creative_id) # Find the owning order via the shadow store: package_ids are # globally unique (upstream line_item ids). - owning_order_id = next( + owning_order_key = next( ( - oid - for oid, state in self._buy_state.items() - if package_id in state.get("packages", {}) + key + for key, state in self._buy_state.items() + if key[0] == self._account_scope(ctx) + and package_id in state.get("packages", {}) ), None, ) - if owning_order_id is None: + if owning_order_key is None: continue + owning_order_id = owning_order_key[1] await upstream_helpers.attach_creative( client, network_code=network_code, @@ -1629,7 +1719,7 @@ async def sync_creatives( line_item_id=package_id, creative_id=upstream_creative_id, ) - pkg_state = self._buy_state[owning_order_id]["packages"].setdefault( + pkg_state = self._buy_state[owning_order_key]["packages"].setdefault( package_id, {"canceled": False, "paused": False} ) existing = list(pkg_state.get("creative_assignments") or []) @@ -1674,6 +1764,9 @@ async def get_media_buy_delivery( client = self._client(ctx) for order_id in media_buy_ids: try: + order_meta = await self._get_owned_order( + ctx, client, network_code=network_code, order_id=order_id + ) upstream_row = await upstream_helpers.get_delivery( client, network_code=network_code, order_id=order_id ) @@ -1688,21 +1781,9 @@ async def get_media_buy_delivery( # the order so we project the correct AdCP MediaBuyStatus # — completed / canceled / rejected buys would otherwise # all surface as 'active' to the buyer. - try: - order_meta = await upstream_helpers.get_order( - client, network_code=network_code, order_id=order_id - ) - upstream_status = order_meta.get("status", "") - except AdcpError as exc: - if exc.code == "MEDIA_BUY_NOT_FOUND": - # Delivery row exists but order is gone — odd, - # surface as 'active' so the row is at least - # well-formed; the operator's audit log will catch it. - upstream_status = "" - else: - raise + upstream_status = order_meta.get("status", "") wire_status = _DELIVERY_STATUS_MAP.get(upstream_status, "active") - buy_state = self._buy_state.get(order_id, {}) + buy_state = self._buy_state.get(self._buy_key(ctx, order_id), {}) if buy_state.get("canceled"): wire_status = "canceled" elif buy_state.get("paused"): @@ -1775,7 +1856,10 @@ async def get_media_buys( # but a single network can host multiple advertisers under the # same network_code — our AdCP account maps to one of them). upstream_orders = [ - o for o in payload.get("orders", []) if o.get("advertiser_id") == advertiser_id + o + for o in payload.get("orders", []) + if o.get("advertiser_id") == advertiser_id + and self._buy_key(ctx, str(o.get("order_id"))) in self._buy_state ] # Narrow to the requested media_buy_ids when the buyer supplied # them. Storyboards chain get_media_buys after create with the @@ -1789,7 +1873,8 @@ async def get_media_buys( media_buys: list[dict[str, Any]] = [] for order in page: order_id = order["order_id"] - buy_state = self._buy_state.get(order_id, {}) + buy_key = self._buy_key(ctx, order_id) + buy_state = self._buy_state[buy_key] wire_status = _DELIVERY_STATUS_MAP.get(order.get("status", ""), "active") if buy_state.get("canceled"): wire_status = "canceled" @@ -1817,7 +1902,7 @@ async def get_media_buys( "media_buy_id": order_id, "status": wire_status, "confirmed_at": _order_confirmed_at(order), - "revision": self._buy_revisions.setdefault(order_id, 1), + "revision": self._buy_revisions.setdefault(buy_key, 1), "currency": order.get("currency", "USD"), "total_budget": float(order.get("budget", 0.0)), "packages": packages, @@ -1910,6 +1995,9 @@ async def provide_performance_feedback( ], } client = self._client(ctx) + await self._get_owned_order( + ctx, client, network_code=network_code, order_id=req.media_buy_id + ) await upstream_helpers.post_conversions( client, network_code=network_code, @@ -1993,7 +2081,10 @@ async def list_creatives( filters = getattr(req, "filters", None) wanted_ids = list(getattr(filters, "creative_ids", None) or []) if filters else [] if wanted_ids: - upstream_wanted = {self._creative_id_map.get(cid, cid) for cid in wanted_ids} + account_scope = self._account_scope(ctx) + upstream_wanted = { + self._creative_id_map.get((account_scope, cid), cid) for cid in wanted_ids + } upstream_creatives = [ c for c in upstream_creatives if c.get("creative_id") in upstream_wanted ] @@ -2038,7 +2129,7 @@ async def list_creatives( # owns the mapping; falls back to the upstream id when the # creative was synced outside this seller instance. "creative_id": self._creative_id_reverse.get( - c["creative_id"], c["creative_id"] + (self._account_scope(ctx), c["creative_id"]), c["creative_id"] ), "name": c["name"], "format_kind": format_kind, diff --git a/examples/v3_reference_seller/tests/test_smoke.py b/examples/v3_reference_seller/tests/test_smoke.py index b65022474..1af9eaf74 100644 --- a/examples/v3_reference_seller/tests/test_smoke.py +++ b/examples/v3_reference_seller/tests/test_smoke.py @@ -11,6 +11,7 @@ import sys from pathlib import Path +from unittest.mock import AsyncMock, MagicMock import pytest @@ -32,6 +33,11 @@ def test_models_import_and_declare_tables() -> None: for cls in (Tenant, BuyerAgent, Account): assert cls.__tablename__ in table_names + api_key_index = next( + index for index in BuyerAgent.__table__.indexes if index.name == "buyer_agents_api_key_uidx" + ) + assert api_key_index.unique is True + def test_platform_satisfies_decisioning_protocol() -> None: """The platform impl exists and can be inspected without an @@ -159,6 +165,38 @@ async def test_buyer_registry_returns_none_without_tenant() -> None: assert await registry.resolve_by_credential(cred) is None +@pytest.mark.asyncio +async def test_buyer_registry_denies_ambiguous_legacy_credential( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Pre-index duplicate credentials fail closed instead of raising.""" + from types import SimpleNamespace + + from src.buyer_registry import TenantScopedBuyerAgentRegistry + + from adcp.decisioning import ApiKeyCredential + + result = MagicMock() + result.scalars.return_value.all.return_value = [MagicMock(), MagicMock()] + session = MagicMock() + session.__aenter__ = AsyncMock(return_value=session) + session.__aexit__ = AsyncMock(return_value=None) + session.execute = AsyncMock(return_value=result) + monkeypatch.setattr( + "src.buyer_registry.current_tenant", + lambda: SimpleNamespace(id="tenant-a"), + ) + registry = TenantScopedBuyerAgentRegistry(sessionmaker=MagicMock(return_value=session)) + + resolved = await registry.resolve_by_credential( + ApiKeyCredential(kind="api_key", key_id="legacy-duplicate") + ) + + assert resolved is None + statement = session.execute.await_args.args[0] + assert statement._limit_clause.value == 2 # noqa: SLF001 - query safety assertion + + def test_platform_default_does_not_advertise_webhook_signing() -> None: """Out-of-the-box, the reference seller advertises no webhook-signing capability — the constructor flag is opt-in. Boot @@ -188,3 +226,66 @@ def test_platform_advertises_webhook_signing_when_alg_passed() -> None: assert ws.profile == "adcp/webhook-signing/v1" assert ws.algorithms is not None assert [a.value for a in ws.algorithms] == ["ed25519"] + + +@pytest.mark.asyncio +@pytest.mark.parametrize("authenticated_tenant", ["tenant-a", None]) +async def test_bearer_context_rejects_cross_tenant_rebinding( + monkeypatch: pytest.MonkeyPatch, + authenticated_tenant: str | None, +) -> None: + """A mismatched or absent token tenant cannot be rebound to the host.""" + import src.app as app_module + + from adcp.decisioning import AdcpError + from adcp.server import ToolContext + from adcp.server.auth import ( + AUTHENTICATED_TENANT_METADATA_KEY, + ROUTED_TENANT_METADATA_KEY, + ) + + monkeypatch.setattr( + app_module, + "auth_context_factory", + lambda _meta: ToolContext( + tenant_id=authenticated_tenant, + metadata={ + AUTHENTICATED_TENANT_METADATA_KEY: authenticated_tenant, + ROUTED_TENANT_METADATA_KEY: "tenant-b", + }, + ), + ) + context = app_module._build_context_factory()(object()) + assert context.tenant_id == "tenant-b" + + with pytest.raises(AdcpError) as excinfo: + await app_module.enforce_authenticated_tenant("get_products", {}, context, AsyncMock()) + assert excinfo.value.code == "PERMISSION_DENIED" + + +@pytest.mark.asyncio +async def test_token_loader_rejects_duplicate_bearer_identifiers() -> None: + """A bearer value must resolve to exactly one buyer and tenant.""" + import src.app as app_module + + rows = [ + MagicMock( + api_key_id="duplicate", + agent_url="https://a.example/", + tenant_id="tenant-a", + ), + MagicMock( + api_key_id="duplicate", + agent_url="https://b.example/", + tenant_id="tenant-b", + ), + ] + result = MagicMock() + result.scalars.return_value = rows + session = MagicMock() + session.__aenter__ = AsyncMock(return_value=session) + session.__aexit__ = AsyncMock(return_value=None) + session.execute = AsyncMock(return_value=result) + + with pytest.raises(RuntimeError, match="Duplicate buyer-agent api_key_id"): + await app_module._load_token_map(MagicMock(return_value=session)) diff --git a/examples/v3_reference_seller/tests/test_smoke_broadening.py b/examples/v3_reference_seller/tests/test_smoke_broadening.py index 1d7fe7ca5..29847860c 100644 --- a/examples/v3_reference_seller/tests/test_smoke_broadening.py +++ b/examples/v3_reference_seller/tests/test_smoke_broadening.py @@ -121,10 +121,11 @@ def test_capabilities_claim_both_sales_specialisms() -> None: def test_storyboard_legacy_format_converter_preserves_the_exact_tuple() -> None: from concurrent.futures import ThreadPoolExecutor + from src.platform import V3ReferenceSeller + from adcp.canonical_formats import normalize_legacy_creative_request from adcp.decisioning import InMemoryTaskRegistry from adcp.decisioning.handler import PlatformHandler - from src.platform import V3ReferenceSeller legacy = { "agent_url": "https://reference.adcp.org", @@ -212,7 +213,6 @@ async def test_account_store_upsert_creates_then_updates_and_strips_bank( :func:`to_wire_sync_accounts_row`. Bank details MUST round-trip into the persisted row but MUST NOT appear on the wire-projected response.""" - import src.platform as platform_module from src.models import BuyerAgent as BuyerAgentRow from src.platform import V3ReferenceSeller @@ -220,6 +220,7 @@ async def test_account_store_upsert_creates_then_updates_and_strips_bank( from adcp.decisioning.account_projection import to_wire_sync_accounts_row from adcp.decisioning.accounts import ResolveContext from adcp.types import SyncAccountsRequest + from src import platform as platform_module bank_block = { "account_holder": "Pinnacle Media LLC", @@ -319,7 +320,6 @@ async def test_account_store_list_strips_bank_details( bank block. Mirrors how the dispatch shim wraps the upstream's response. """ - import src.platform as platform_module from src.models import Account as AccountRow from src.models import BuyerAgent as BuyerAgentRow from src.platform import V3ReferenceSeller @@ -327,6 +327,7 @@ async def test_account_store_list_strips_bank_details( from adcp.decisioning import AuthInfo from adcp.decisioning.account_projection import to_wire_account from adcp.decisioning.accounts import ResolveContext + from src import platform as platform_module bank_block = { "account_holder": "Pinnacle Media LLC", @@ -396,6 +397,146 @@ class _Tenant: ), f"bank details leaked through to_wire_account projection: {wire}" +@pytest.mark.asyncio +async def test_account_store_explicit_id_is_bound_to_authenticated_buyer( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """An account id owned by another buyer is indistinguishable from missing.""" + from adcp.decisioning import AdcpError, AuthInfo + from src import platform as platform_module + + buyer_result = MagicMock() + buyer_result.scalar_one_or_none.return_value = MagicMock(id="ba_caller") + missing_account_result = MagicMock() + missing_account_result.scalar_one_or_none.return_value = None + session = MagicMock() + session.__aenter__ = AsyncMock(return_value=session) + session.__aexit__ = AsyncMock(return_value=None) + session.execute = AsyncMock(side_effect=[buyer_result, missing_account_result]) + + class _Tenant: + id = "t_acme" + + monkeypatch.setattr(platform_module, "current_tenant", lambda: _Tenant()) + store = platform_module._make_account_store( # noqa: SLF001 - example integration test + MagicMock(return_value=session), mock_upstream_url="http://up.test" + ) + auth = AuthInfo(kind="anonymous", principal="https://caller.example/") + with pytest.raises(AdcpError) as excinfo: + await store.resolve({"account_id": "foreign-account"}, auth) + assert excinfo.value.code == "ACCOUNT_NOT_FOUND" + account_query = session.execute.await_args_list[1].args[0] + compiled = str(account_query.compile(compile_kwargs={"literal_binds": True})) + assert "accounts.buyer_agent_id = 'ba_caller'" in compiled + assert "accounts.account_id = 'foreign-account'" in compiled + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "account_ref", + [ + {"account_id": "account-1"}, + {"brand": {"domain": "brand.example"}, "operator": "operator.example"}, + ], +) +async def test_account_store_without_principal_uses_correctable_auth_missing( + monkeypatch: pytest.MonkeyPatch, + account_ref: dict[str, Any], +) -> None: + from adcp.decisioning import AdcpError + from src import platform as platform_module + + class _Tenant: + id = "t_acme" + + monkeypatch.setattr(platform_module, "current_tenant", lambda: _Tenant()) + store = platform_module._make_account_store( # noqa: SLF001 - example integration test + MagicMock(), mock_upstream_url="http://up.test" + ) + + with pytest.raises(AdcpError) as excinfo: + await store.resolve(account_ref, None) + + assert excinfo.value.code == "AUTH_MISSING" + assert excinfo.value.recovery == "correctable" + + +@pytest.mark.asyncio +async def test_account_store_upsert_cannot_overwrite_another_buyers_account( + monkeypatch: pytest.MonkeyPatch, +) -> None: + from src.models import Account as AccountRow + + from adcp.decisioning import AuthInfo + from adcp.decisioning.accounts import ResolveContext + from adcp.types import SyncAccountsRequest + from src import platform as platform_module + + buyer_result = MagicMock() + buyer_result.scalar_one_or_none.return_value = MagicMock(id="ba_caller") + foreign = AccountRow( + id="a_foreign", + tenant_id="t_acme", + buyer_agent_id="ba_other", + account_id="acme.example::operator.example", + name="Foreign", + status="active", + sandbox=False, + ) + existing_result = MagicMock() + existing_result.scalar_one_or_none.return_value = foreign + missing_result = MagicMock() + missing_result.scalar_one_or_none.return_value = None + session = MagicMock() + session.__aenter__ = AsyncMock(return_value=session) + session.__aexit__ = AsyncMock(return_value=None) + session.begin = MagicMock(return_value=session) + session.execute = AsyncMock(side_effect=[buyer_result, missing_result, existing_result]) + + class _Tenant: + id = "t_acme" + + monkeypatch.setattr(platform_module, "current_tenant", lambda: _Tenant()) + platform = platform_module.V3ReferenceSeller( + sessionmaker=MagicMock(return_value=session), upstream_api_key="test-key" + ) + req = SyncAccountsRequest.model_validate( + { + "idempotency_key": "k_" + "a" * 18, + "accounts": [ + { + "brand": {"domain": "one.example"}, + "operator": "operator.example", + "billing": "operator", + }, + { + "brand": {"domain": "acme.example"}, + "operator": "operator.example", + "billing": "operator", + }, + ], + } + ) + ctx = ResolveContext( + auth_info=AuthInfo(kind="anonymous", principal="https://caller.example/"), + tool_name="sync_accounts", + ) + rows = await platform.accounts.upsert(list(req.accounts), ctx) + assert [row.action for row in rows] == ["created", "failed"] + assert rows[1].status == "rejected" + assert rows[1].errors == [ + { + "code": "ACCOUNT_NOT_FOUND", + "message": "Account is not visible to the authenticated buyer agent.", + "recovery": "terminal", + "field": "accounts[1]", + } + ] + added = session.add.call_args.args[0] + assert added.account_id == "one.example::operator.example" + assert foreign.buyer_agent_id == "ba_other" + + # --------------------------------------------------------------------------- # Translator-pattern HTTP plumbing — upstream is called via upstream_for() # --------------------------------------------------------------------------- @@ -463,6 +604,27 @@ def _platform_with_upstream() -> Any: ) +def _seed_owned_buy( + platform: Any, + ctx: Any, + order_id: str, + *, + packages: dict[str, Any] | None = None, +) -> None: + """Seed the reference seller's account-owned shadow state. + + Real requests establish this binding through ``create_media_buy``; + translator unit tests that start from an existing upstream order must + declare the same ownership explicitly. + """ + buy_key = platform._buy_key(ctx, order_id) # noqa: SLF001 - ownership test helper + platform._buy_state[buy_key] = { # noqa: SLF001 - ownership test helper + "packages": packages or {}, + "canceled": False, + "paused": False, + } + + _LINE_ITEM_COUNTER = {"n": 0} @@ -803,6 +965,18 @@ async def test_create_media_buy_echoes_packages_with_seller_minted_ids( assert result.media_buy_status is not None assert result.media_buy_status.value == "pending_start" + # The framework's response scrubber must retain nested model identity. + # The legacy 3.1 response projector dispatches on Package models; an + # unchecked model_copy(update=...) turns these into dicts and leaves the + # canonical format_option_refs shape on the legacy wire response. + from adcp.decisioning.account_projection import strip_credentials_from_wire_result + + scrubbed = strip_credentials_from_wire_result("create_media_buy", result) + assert isinstance(scrubbed, CreateMediaBuySuccessResponse) + assert type(scrubbed.packages[0]) is type(result.packages[0]) + assert scrubbed.packages[0].targeting_overlay is not None + assert scrubbed.packages[0].targeting_overlay.property_list is not None + @pytest.mark.asyncio @respx.mock(base_url=_RESPX_BASE_URL) @@ -963,7 +1137,11 @@ async def test_update_media_buy_cancel_marks_local_state(respx_mock: Any) -> Non respx_mock.get("/v1/orders/ord_test").mock( return_value=httpx.Response( 200, - json={"order_id": "ord_test", "status": "active"}, + json={ + "order_id": "ord_test", + "status": "active", + "advertiser_id": "adv_volta_motors", + }, ) ) respx_mock.get("/v1/orders/ord_test/lineitems").mock( @@ -972,6 +1150,7 @@ async def test_update_media_buy_cancel_marks_local_state(respx_mock: Any) -> Non platform = _platform_with_upstream() ctx = _build_ctx() + _seed_owned_buy(platform, ctx, "ord_test") patch = UpdateMediaBuyRequest.model_validate( { "account": {"account_id": "signed-buyer-main"}, @@ -1019,6 +1198,7 @@ async def test_update_media_buy_unknown_media_buy_id_raises_not_found( platform = _platform_with_upstream() ctx = _build_ctx() + _seed_owned_buy(platform, ctx, "ord_test", packages={"li_known": {}}) patch = UpdateMediaBuyRequest.model_validate( { "account": {"account_id": "signed-buyer-main"}, @@ -1044,7 +1224,11 @@ async def test_update_media_buy_unknown_package_id_raises_not_found( respx_mock.get("/v1/orders/ord_test").mock( return_value=httpx.Response( 200, - json={"order_id": "ord_test", "status": "active"}, + json={ + "order_id": "ord_test", + "status": "active", + "advertiser_id": "adv_volta_motors", + }, ) ) respx_mock.get("/v1/orders/ord_test/lineitems").mock( @@ -1056,6 +1240,7 @@ async def test_update_media_buy_unknown_package_id_raises_not_found( platform = _platform_with_upstream() ctx = _build_ctx() + _seed_owned_buy(platform, ctx, "ord_test", packages={"li_known": {}}) patch = UpdateMediaBuyRequest.model_validate( { "account": {"account_id": "signed-buyer-main"}, @@ -1081,7 +1266,11 @@ async def test_update_media_buy_affected_packages_echo_list_agent_urls( respx_mock.get("/v1/orders/ord_test").mock( return_value=httpx.Response( 200, - json={"order_id": "ord_test", "status": "delivering"}, + json={ + "order_id": "ord_test", + "status": "delivering", + "advertiser_id": "adv_volta_motors", + }, ) ) respx_mock.get("/v1/orders/ord_test/lineitems").mock( @@ -1092,12 +1281,13 @@ async def test_update_media_buy_affected_packages_echo_list_agent_urls( ) platform = _platform_with_upstream() - platform._buy_state["ord_test"] = { # noqa: SLF001 - example shadow-store regression test - "packages": {"li_known": {"canceled": False, "paused": False}}, - "canceled": False, - "paused": False, - } ctx = _build_ctx() + _seed_owned_buy( + platform, + ctx, + "ord_test", + packages={"li_known": {"canceled": False, "paused": False}}, + ) patch = UpdateMediaBuyRequest.model_validate( { "account": {"account_id": "signed-buyer-main"}, @@ -1123,7 +1313,12 @@ async def test_update_media_buy_affected_packages_echo_list_agent_urls( result = await platform.update_media_buy("ord_test", patch, ctx) assert isinstance(result, UpdateMediaBuySuccessResponse) - payload = result.model_dump(mode="json", exclude_none=True) + from adcp.decisioning.account_projection import strip_credentials_from_wire_result + + scrubbed = strip_credentials_from_wire_result("update_media_buy", result) + assert isinstance(scrubbed, UpdateMediaBuySuccessResponse) + assert type(scrubbed.affected_packages[0]) is type(result.affected_packages[0]) + payload = scrubbed.model_dump(mode="json", exclude_none=True) targeting = payload["affected_packages"][0]["targeting_overlay"] assert targeting["property_list"]["agent_url"] == ("https://governance.pinnacle-agency.example") assert targeting["collection_list"]["agent_url"] == ( @@ -1264,6 +1459,7 @@ async def test_get_media_buys_filters_by_advertiser_id(respx_mock: Any) -> None: ) platform = _platform_with_upstream() ctx = _build_ctx() + _seed_owned_buy(platform, ctx, "ord_volta_1") resp = await platform.get_media_buys(GetMediaBuysRequest(), ctx) payload = resp.model_dump(mode="json", exclude_none=True) media_buys = payload["media_buys"] @@ -1320,6 +1516,7 @@ async def test_get_media_buy_delivery_translates_upstream_report( ) platform = _platform_with_upstream() ctx = _build_ctx() + _seed_owned_buy(platform, ctx, "ord_1") req = GetMediaBuyDeliveryRequest.model_validate({"media_buy_ids": ["ord_1"]}) resp = await platform.get_media_buy_delivery(req, ctx) payload = resp.model_dump(mode="json", exclude_none=True) @@ -1347,8 +1544,15 @@ async def test_provide_performance_feedback_posts_capi_conversion( json={"order_id": "ord_1", "events_received": 1, "events_deduplicated": 0}, ) ) + respx_mock.get("/v1/orders/ord_1").mock( + return_value=httpx.Response( + 200, + json={"order_id": "ord_1", "advertiser_id": "adv_volta_motors"}, + ) + ) platform = _platform_with_upstream() ctx = _build_ctx() + _seed_owned_buy(platform, ctx, "ord_1") req = ProvidePerformanceFeedbackRequest.model_validate( { "idempotency_key": "k_" + "p" * 18, @@ -1413,7 +1617,7 @@ async def test_provide_performance_feedback_404_translates_to_media_buy_not_foun from adcp.decisioning import AdcpError from adcp.types import ProvidePerformanceFeedbackRequest - respx_mock.post("/v1/orders/ord_missing/conversions").mock( + respx_mock.get("/v1/orders/ord_missing").mock( return_value=httpx.Response(404, json={"code": "ORDER_NOT_FOUND", "message": "missing"}) ) platform = _platform_with_upstream() @@ -1515,6 +1719,221 @@ async def test_list_creative_formats_is_static_no_upstream_call() -> None: assert respx_mock.calls.call_count == 0 +@pytest.mark.asyncio +@respx.mock(base_url=_RESPX_BASE_URL) +async def test_update_media_buy_rejects_foreign_advertiser_order(respx_mock: Any) -> None: + from adcp.decisioning import AdcpError + from adcp.types import UpdateMediaBuyRequest + + respx_mock.get("/v1/orders/ord_foreign").mock( + return_value=httpx.Response( + 200, + json={"order_id": "ord_foreign", "advertiser_id": "adv_other"}, + ) + ) + platform = _platform_with_upstream() + req = UpdateMediaBuyRequest.model_validate( + { + "account": {"account_id": "signed-buyer-main"}, + "media_buy_id": "ord_foreign", + "idempotency_key": "k_" + "u" * 18, + "paused": True, + } + ) + with pytest.raises(AdcpError) as excinfo: + await platform.update_media_buy("ord_foreign", req, _build_ctx()) + assert excinfo.value.code == "MEDIA_BUY_NOT_FOUND" + assert not any(call.request.url.path.endswith("/lineitems") for call in respx_mock.calls) + + +@pytest.mark.asyncio +@respx.mock(base_url=_RESPX_BASE_URL) +async def test_update_media_buy_rejects_other_account_with_shared_advertiser( + respx_mock: Any, +) -> None: + """Advertiser identity alone is not an account ownership boundary.""" + from dataclasses import replace + + from adcp.decisioning import AdcpError + from adcp.types import UpdateMediaBuyRequest + + respx_mock.get("/v1/orders/ord_shared").mock( + return_value=httpx.Response( + 200, + json={"order_id": "ord_shared", "advertiser_id": "adv_volta_motors"}, + ) + ) + platform = _platform_with_upstream() + owner_ctx = _build_ctx() + _seed_owned_buy(platform, owner_ctx, "ord_shared") + assert owner_ctx.account is not None + other_ctx = replace(owner_ctx, account=replace(owner_ctx.account, id="a_other_buyer")) + req = UpdateMediaBuyRequest.model_validate( + { + "account": {"account_id": "other-buyer"}, + "media_buy_id": "ord_shared", + "idempotency_key": "k_" + "o" * 18, + "paused": True, + } + ) + + with pytest.raises(AdcpError) as excinfo: + await platform.update_media_buy("ord_shared", req, other_ctx) + assert excinfo.value.code == "MEDIA_BUY_NOT_FOUND" + assert not any(call.request.url.path.endswith("/lineitems") for call in respx_mock.calls) + + +@pytest.mark.asyncio +@respx.mock(base_url=_RESPX_BASE_URL) +async def test_foreign_and_missing_media_buy_errors_are_identical(respx_mock: Any) -> None: + """Ownership hiding covers the complete public error envelope.""" + from adcp.decisioning import AdcpError + from adcp.types import UpdateMediaBuyRequest + + route = respx_mock.get("/v1/orders/ord_hidden") + route.side_effect = [ + httpx.Response( + 200, + json={"order_id": "ord_hidden", "advertiser_id": "adv_other"}, + ), + httpx.Response(404), + ] + platform = _platform_with_upstream() + ctx = _build_ctx() + _seed_owned_buy(platform, ctx, "ord_hidden") + errors = [] + for suffix in ("f", "m"): + req = UpdateMediaBuyRequest.model_validate( + { + "account": {"account_id": "signed-buyer-main"}, + "media_buy_id": "ord_hidden", + "idempotency_key": "k_" + suffix * 18, + "paused": True, + } + ) + with pytest.raises(AdcpError) as excinfo: + await platform.update_media_buy("ord_hidden", req, ctx) + errors.append(excinfo.value.to_wire()) + + assert errors[0] == errors[1] + + +@pytest.mark.asyncio +@respx.mock(base_url=_RESPX_BASE_URL) +async def test_order_missing_ownership_metadata_is_upstream_failure(respx_mock: Any) -> None: + from adcp.decisioning import AdcpError + from adcp.types import UpdateMediaBuyRequest + + respx_mock.get("/v1/orders/ord_malformed").mock( + return_value=httpx.Response(200, json={"order_id": "ord_malformed"}) + ) + platform = _platform_with_upstream() + ctx = _build_ctx() + _seed_owned_buy(platform, ctx, "ord_malformed") + req = UpdateMediaBuyRequest.model_validate( + { + "account": {"account_id": "signed-buyer-main"}, + "media_buy_id": "ord_malformed", + "idempotency_key": "k_" + "m" * 18, + "paused": True, + } + ) + + with pytest.raises(AdcpError) as excinfo: + await platform.update_media_buy("ord_malformed", req, ctx) + + assert excinfo.value.code == "SERVICE_UNAVAILABLE" + assert excinfo.value.recovery == "transient" + + +@pytest.mark.asyncio +@respx.mock(base_url=_RESPX_BASE_URL) +async def test_delivery_omits_foreign_advertiser_order(respx_mock: Any) -> None: + from adcp.types import GetMediaBuyDeliveryRequest + + respx_mock.get("/v1/orders/ord_foreign").mock( + return_value=httpx.Response( + 200, + json={"order_id": "ord_foreign", "advertiser_id": "adv_other"}, + ) + ) + platform = _platform_with_upstream() + req = GetMediaBuyDeliveryRequest.model_validate({"media_buy_ids": ["ord_foreign"]}) + response = await platform.get_media_buy_delivery(req, _build_ctx()) + assert response.media_buy_deliveries == [] + assert not any(call.request.url.path.endswith("/delivery") for call in respx_mock.calls) + + +@pytest.mark.asyncio +@respx.mock(base_url=_RESPX_BASE_URL) +async def test_performance_feedback_rejects_foreign_advertiser_order( + respx_mock: Any, +) -> None: + from adcp.decisioning import AdcpError + from adcp.types import ProvidePerformanceFeedbackRequest + + respx_mock.get("/v1/orders/ord_foreign").mock( + return_value=httpx.Response( + 200, + json={"order_id": "ord_foreign", "advertiser_id": "adv_other"}, + ) + ) + platform = _platform_with_upstream() + req = ProvidePerformanceFeedbackRequest.model_validate( + { + "idempotency_key": "k_" + "f" * 18, + "media_buy_id": "ord_foreign", + "metric_type": "conversion_rate", + "performance_index": 1.0, + "measurement_period": { + "start": "2026-04-01T00:00:00Z", + "end": "2026-04-30T23:59:59Z", + }, + } + ) + with pytest.raises(AdcpError) as excinfo: + await platform.provide_performance_feedback(req, _build_ctx()) + assert excinfo.value.code == "MEDIA_BUY_NOT_FOUND" + assert not any(call.request.method == "POST" for call in respx_mock.calls) + + +@pytest.mark.asyncio +@respx.mock(base_url=_RESPX_BASE_URL) +async def test_creative_id_mapping_is_scoped_to_account(respx_mock: Any) -> None: + from adcp.types import SyncCreativesRequest + + uploads = iter(["up_account_a", "up_account_b"]) + + def upload(_: httpx.Request) -> httpx.Response: + return httpx.Response(201, json={"creative_id": next(uploads)}) + + route = respx_mock.post("/v1/creatives").mock(side_effect=upload) + platform = _platform_with_upstream() + req = SyncCreativesRequest.model_validate( + { + "account": {"account_id": "account"}, + "idempotency_key": "k_" + "c" * 18, + "creatives": [ + { + "creative_id": "shared-id", + "name": "Creative", + "format_kind": "image", + "assets": {}, + } + ], + } + ) + ctx_a = _build_ctx() + ctx_b = _build_ctx() + ctx_b.account.id = "a_acme_2" + ctx_b.account.metadata["account_id"] = "other-account" + ctx_b.account.metadata["advertiser_id"] = "adv_other" + + await platform.sync_creatives(req, ctx_a) + await platform.sync_creatives(req, ctx_b) + assert route.call_count == 2 + + @pytest.mark.asyncio async def test_account_loader_rejects_account_missing_upstream_routing( monkeypatch: pytest.MonkeyPatch, @@ -1524,11 +1943,11 @@ async def test_account_loader_rejects_account_missing_upstream_routing( AccountStore rejects with ``SERVICE_UNAVAILABLE`` (transient — the fix is upstream onboarding) rather than dispatching to a method that would 500 on upstream call.""" - import src.platform as platform_module from src.models import Account as AccountRow from src.platform import _make_account_store - from adcp.decisioning import AdcpError + from adcp.decisioning import AdcpError, AuthInfo + from src import platform as platform_module bad_row = AccountRow( id="a_bad", @@ -1541,12 +1960,14 @@ async def test_account_loader_rejects_account_missing_upstream_routing( sandbox=False, ext=None, ) + buyer_result = MagicMock() + buyer_result.scalar_one_or_none = MagicMock(return_value=MagicMock(id="ba_x")) result = MagicMock() result.scalar_one_or_none = MagicMock(return_value=bad_row) session = MagicMock() session.__aenter__ = AsyncMock(return_value=session) session.__aexit__ = AsyncMock(return_value=None) - session.execute = AsyncMock(return_value=result) + session.execute = AsyncMock(side_effect=[buyer_result, result]) sessionmaker = MagicMock(return_value=session) class _Tenant: @@ -1556,7 +1977,10 @@ class _Tenant: store = _make_account_store(sessionmaker, mock_upstream_url="http://up.test") with pytest.raises(AdcpError) as excinfo: - await store.resolve({"account_id": "bad-acct"}) + await store.resolve( + {"account_id": "bad-acct"}, + AuthInfo(kind="anonymous", principal="https://signed-buyer.example/"), + ) assert excinfo.value.code == "SERVICE_UNAVAILABLE" assert excinfo.value.recovery == "transient" @@ -1570,10 +1994,12 @@ async def test_account_loader_returns_mock_mode_with_upstream_url( so the framework's ``upstream_for(ctx)`` routes the adapter at the mock-server fixture URL. """ - import src.platform as platform_module from src.models import Account as AccountRow from src.platform import _make_account_store + from adcp.decisioning import AuthInfo + from src import platform as platform_module + good_row = AccountRow( id="a_good", tenant_id="t_acme", @@ -1585,12 +2011,14 @@ async def test_account_loader_returns_mock_mode_with_upstream_url( sandbox=False, ext={"network_code": "net_premium_us", "advertiser_id": "adv_volta_motors"}, ) + buyer_result = MagicMock() + buyer_result.scalar_one_or_none = MagicMock(return_value=MagicMock(id="ba_x")) result = MagicMock() result.scalar_one_or_none = MagicMock(return_value=good_row) session = MagicMock() session.__aenter__ = AsyncMock(return_value=session) session.__aexit__ = AsyncMock(return_value=None) - session.execute = AsyncMock(return_value=result) + session.execute = AsyncMock(side_effect=[buyer_result, result]) sessionmaker = MagicMock(return_value=session) class _Tenant: @@ -1599,7 +2027,10 @@ class _Tenant: monkeypatch.setattr(platform_module, "current_tenant", lambda: _Tenant()) store = _make_account_store(sessionmaker, mock_upstream_url="http://127.0.0.1:4503") - account = await store.resolve({"account_id": "good-acct"}) + account = await store.resolve( + {"account_id": "good-acct"}, + AuthInfo(kind="anonymous", principal="https://signed-buyer.example/"), + ) assert account.mode == "mock" assert account.metadata["mock_upstream_url"] == "http://127.0.0.1:4503" # Routing data still flows through metadata so platform methods @@ -1632,7 +2063,7 @@ async def test_get_products_401_translates_to_auth_required(respx_mock: Any) -> GetProductsRequest.model_validate({"buying_mode": "wholesale"}), ctx ) assert excinfo.value.code == "AUTH_REQUIRED" - assert excinfo.value.recovery == "terminal" + assert excinfo.value.recovery == "correctable" @pytest.mark.asyncio @@ -2062,6 +2493,7 @@ async def test_get_media_buy_delivery_projects_completed_status( ) platform = _platform_with_upstream() ctx = _build_ctx() + _seed_owned_buy(platform, ctx, "ord_done") req = GetMediaBuyDeliveryRequest.model_validate({"media_buy_ids": ["ord_done"]}) resp = await platform.get_media_buy_delivery(req, ctx) payload = resp.model_dump(mode="json", exclude_none=True) @@ -2109,6 +2541,7 @@ async def test_get_media_buy_delivery_projects_canceled_status( ) platform = _platform_with_upstream() ctx = _build_ctx() + _seed_owned_buy(platform, ctx, "ord_killed") req = GetMediaBuyDeliveryRequest.model_validate({"media_buy_ids": ["ord_killed"]}) resp = await platform.get_media_buy_delivery(req, ctx) payload = resp.model_dump(mode="json", exclude_none=True) diff --git a/examples/v3_reference_seller/tests/test_unique_api_key_migration.py b/examples/v3_reference_seller/tests/test_unique_api_key_migration.py new file mode 100644 index 000000000..142f857eb --- /dev/null +++ b/examples/v3_reference_seller/tests/test_unique_api_key_migration.py @@ -0,0 +1,71 @@ +"""Regression tests for the bearer-credential uniqueness migration.""" + +from __future__ import annotations + +import importlib.util +from pathlib import Path +from unittest.mock import MagicMock, patch + +import pytest + +_MIGRATION = ( + Path(__file__).resolve().parents[1] / "alembic" / "versions" / "0003_unique_buyer_api_key.py" +) + + +def _load_migration(): + spec = importlib.util.spec_from_file_location("unique_buyer_api_key_migration", _MIGRATION) + assert spec is not None and spec.loader is not None + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +def test_upgrade_rejects_legacy_duplicates_before_index_changes() -> None: + migration = _load_migration() + result = MagicMock() + result.scalar_one.return_value = 2 + connection = MagicMock() + connection.execute.return_value = result + + with ( + patch.object(migration.context, "is_offline_mode", return_value=False), + patch.object(migration.op, "get_bind", return_value=connection), + patch.object(migration.op, "drop_index") as drop_index, + ): + with pytest.raises(RuntimeError, match="duplicated credential identifier"): + migration.upgrade() + drop_index.assert_not_called() + + +def test_upgrade_replaces_legacy_index_with_unique_index() -> None: + migration = _load_migration() + result = MagicMock() + result.scalar_one.return_value = 0 + connection = MagicMock() + connection.execute.return_value = result + + with ( + patch.object(migration.context, "is_offline_mode", return_value=False), + patch.object(migration.op, "get_bind", return_value=connection), + patch.object(migration.op, "drop_index") as drop_index, + patch.object(migration.op, "create_index") as create_index, + ): + migration.upgrade() + + drop_index.assert_called_once_with("buyer_agents_api_key_idx", table_name="buyer_agents") + assert create_index.call_args.kwargs["unique"] is True + + +def test_offline_upgrade_emits_index_changes_without_querying_data() -> None: + migration = _load_migration() + with ( + patch.object(migration.context, "is_offline_mode", return_value=True), + patch.object(migration.op, "get_bind") as get_bind, + patch.object(migration.op, "drop_index"), + patch.object(migration.op, "create_index") as create_index, + ): + migration.upgrade() + + get_bind.assert_not_called() + assert create_index.call_args.kwargs["unique"] is True diff --git a/src/adcp/decisioning/__init__.py b/src/adcp/decisioning/__init__.py index ed5131944..2eced3abb 100644 --- a/src/adcp/decisioning/__init__.py +++ b/src/adcp/decisioning/__init__.py @@ -256,6 +256,7 @@ def create_media_buy( NoAuth, StaticBearer, UpstreamAuth, + UpstreamClientPool, UpstreamHttpClient, create_upstream_http_client, ) @@ -435,6 +436,7 @@ def __init__(self, *args: object, **kwargs: object) -> None: "UnsupportedFeatureError", "UpdateMediaBuyMutation", "UpstreamAuth", + "UpstreamClientPool", "UpstreamHttpClient", "ValidationError", "WorkflowHandoff", diff --git a/src/adcp/decisioning/account_projection.py b/src/adcp/decisioning/account_projection.py index 9d3554884..418c873e4 100644 --- a/src/adcp/decisioning/account_projection.py +++ b/src/adcp/decisioning/account_projection.py @@ -375,6 +375,8 @@ def _scrub_dict(value: dict[str, Any]) -> dict[str, Any]: * ``governance_agents[i].authentication`` — write-only credential. * ``billing_entity.bank`` — write-only bank coordinates. + * ``notification_configs[i].authentication.credentials`` — legacy + webhook bearer/HMAC secret. Walks recursively into nested dicts and lists. Returns a NEW dict — the input is not mutated, so callers (idempotency replay cache, @@ -383,22 +385,62 @@ def _scrub_dict(value: dict[str, Any]) -> dict[str, Any]: out: dict[str, Any] = {} for key, sub in value.items(): if key == "governance_agents" and isinstance(sub, list): - out[key] = [_scrub_governance_agent_dict(a) if isinstance(a, dict) else a for a in sub] - elif key == "billing_entity" and isinstance(sub, dict): - out[key] = {k: v for k, v in _scrub_value(sub).items() if k != "bank"} + out[key] = [ + ( + _scrub_governance_agent_dict(_scrub_value(a)) + if isinstance(_scrub_value(a), dict) + else _scrub_value(a) + ) + for a in sub + ] + elif key == "notification_configs" and isinstance(sub, list): + out[key] = [ + ( + _scrub_notification_config_dict(_scrub_value(config)) + if isinstance(_scrub_value(config), dict) + else _scrub_value(config) + ) + for config in sub + ] + elif key in {"billing_entity", "invoice_recipient"}: + projected = _scrub_value(sub) + if isinstance(projected, dict): + out[key] = {k: v for k, v in projected.items() if k != "bank"} + else: + out[key] = projected elif key == "authorization": authorization = _project_account_authorization(sub) if authorization is not None: out[key] = authorization elif key == "errors" and isinstance(sub, list): - out[key] = [_scrub_error_dict(e) if isinstance(e, dict) else e for e in sub] - elif key == "adcp_error" and isinstance(sub, dict): - out[key] = _scrub_error_dict(sub) + normalized_errors = [_scrub_value(error) for error in sub] + out[key] = [ + _scrub_error_dict(error) if isinstance(error, dict) else error + for error in normalized_errors + ] + elif key == "adcp_error": + normalized_error = _scrub_value(sub) + out[key] = ( + _scrub_error_dict(normalized_error) + if isinstance(normalized_error, dict) + else normalized_error + ) else: out[key] = _scrub_value(sub) return out +def _scrub_notification_config_dict(config: dict[str, Any]) -> dict[str, Any]: + """Strip legacy webhook credentials while preserving the auth scheme.""" + out = {key: _scrub_value(value) for key, value in config.items()} + authentication = out.get("authentication") + if isinstance(authentication, dict): + out["authentication"] = { + key: value for key, value in authentication.items() if key != "credentials" + } + return out + + def _scrub_error_dict(error: dict[str, Any]) -> dict[str, Any]: """Strip private fields from code-specific error details.""" out = {k: _scrub_value(v) for k, v in error.items() if k != "details"} @@ -419,6 +461,8 @@ def _scrub_governance_agent_dict(agent: dict[str, Any]) -> dict[str, Any]: def _scrub_value(value: Any) -> Any: """Recurse into dicts / lists; return primitives unchanged.""" + if hasattr(value, "model_dump"): + value = value.model_dump(mode="json") if isinstance(value, dict): return _scrub_dict(value) if isinstance(value, list): @@ -465,16 +509,22 @@ def strip_credentials_from_wire_result(method_name: str, result: Any) -> Any: return scrubbed if isinstance(result, list): return [_scrub_value(v) for v in result] - # Typed Pydantic response models pass through unchanged — the - # response-side codegen'd shapes don't define ``authentication`` - # on ``GovernanceAgent`` or ``bank`` on the response-side - # ``BusinessEntity``, so the schema enforces the strip - # structurally. Dumping-and-scrubbing a model would force - # downstream callers to lose typed-model identity for no - # security gain. The leak vector is loose dicts and Pydantic - # ``extra='allow'`` models that smuggle credentials past the - # codegen schema; both arrive as ``dict`` after the adopter's - # method returns or via the registry's ``model_dump`` path. + if hasattr(result, "model_dump"): + # Preserve the concrete response model: handler callers rely on + # typed attributes (including nested package/creative models) even + # though credential-bearing values still need a defensive scrub + # before persistence/webhook emission. Update only top-level fields + # whose serialized value actually changed: replacing every field with + # its dumped form loses nested model identity, while revalidating the + # whole response can normalize buyer-supplied lexical values (such as + # adding a trailing slash to an agent URL). + dumped = result.model_dump(mode="python") + scrubbed = _scrub_dict(dumped) + updates = {key: value for key, value in scrubbed.items() if value != dumped.get(key)} + model_copy = getattr(result, "model_copy", None) + if callable(model_copy): + return model_copy(update=updates) + return scrubbed return result diff --git a/src/adcp/decisioning/pg/buyer_agent_registry.py b/src/adcp/decisioning/pg/buyer_agent_registry.py index c36724909..57cb9c9cb 100644 --- a/src/adcp/decisioning/pg/buyer_agent_registry.py +++ b/src/adcp/decisioning/pg/buyer_agent_registry.py @@ -170,7 +170,7 @@ def __init__( raise ImportError(_INSTALL_HINT) if not _is_safe_identifier(table_name): raise ValueError( - "table_name must match [a-z_][a-z0-9_]* (ASCII only), " f"got {table_name!r}" + f"table_name must match [a-z_][a-z0-9_]* (ASCII only), got {table_name!r}" ) self._pool = pool self._table = table_name @@ -190,7 +190,7 @@ def __init__( ) self._sql_select_by_api_key_id = ( f"SELECT {cols} FROM {self._table} " # noqa: S608 - f"WHERE api_key_id = %s" + f"WHERE api_key_id = %s LIMIT 2" ) self._sql_upsert = ( f"INSERT INTO {self._table} (" # noqa: S608 @@ -219,8 +219,12 @@ def __init__( def create_schema(self) -> None: """Create the registry table + indexes for this store's - ``table_name``. Idempotent via ``CREATE ... IF NOT EXISTS``; - safe to call on every app boot. + ``table_name``. Idempotent once credential identifiers are unique. + + Existing deployments with duplicated ``api_key_id`` values fail + before indexes change, with an actionable rotation/removal message. + The legacy non-unique credential index is dropped only after the + replacement unique index exists. The equivalent raw DDL ships at :file:`src/adcp/decisioning/pg/buyer_agent_registry.sql` for @@ -228,7 +232,7 @@ def create_schema(self) -> None: that file uses the canonical ``adcp_buyer_agents`` name. """ table = self._table # already validated at __init__ - ddl = ( + table_ddl = ( f"CREATE TABLE IF NOT EXISTS {table} (" # noqa: S608 — validated f' agent_url TEXT COLLATE "C" PRIMARY KEY,' f" display_name TEXT NOT NULL," @@ -242,13 +246,34 @@ def create_schema(self) -> None: f" created_at TIMESTAMPTZ NOT NULL DEFAULT now()," f" updated_at TIMESTAMPTZ NOT NULL DEFAULT now()" f");" - f"CREATE INDEX IF NOT EXISTS {table}_api_key_id_idx " # noqa: S608 - f" ON {table} (api_key_id) WHERE api_key_id IS NOT NULL;" - f"CREATE INDEX IF NOT EXISTS {table}_status_idx " # noqa: S608 - f" ON {table} (status) WHERE status <> 'active';" + ) + duplicate_preflight = ( # noqa: S608 — validated table name + f"SELECT api_key_id, COUNT(*) FROM {table} " + f"WHERE api_key_id IS NOT NULL GROUP BY api_key_id " + f"HAVING COUNT(*) > 1 LIMIT 1" + ) + unique_index_ddl = ( # noqa: S608 + f"CREATE UNIQUE INDEX IF NOT EXISTS {table}_api_key_id_uidx " + f"ON {table} (api_key_id) WHERE api_key_id IS NOT NULL" + ) + drop_legacy_index_ddl = f"DROP INDEX IF EXISTS {table}_api_key_id_idx" # noqa: S608 + status_index_ddl = ( # noqa: S608 + f"CREATE INDEX IF NOT EXISTS {table}_status_idx " + f"ON {table} (status) WHERE status <> 'active'" ) with self._pool.connection() as conn, conn.cursor() as cur: - cur.execute(ddl) + cur.execute(table_ddl) + cur.execute(duplicate_preflight) + duplicate = cur.fetchall() + if duplicate: + raise RuntimeError( + f"Cannot enforce {table}.api_key_id uniqueness: duplicated credential " + "identifiers exist. Rotate or remove duplicate bearer credentials, then " + "rerun create_schema()." + ) + cur.execute(unique_index_ddl) + cur.execute(drop_legacy_index_ddl) + cur.execute(status_index_ddl) # ----- BuyerAgentRegistry Protocol -------------------------------- @@ -496,8 +521,7 @@ def _notify_mutation(self, op: str, agent_url: str) -> None: observer(op, agent_url) except Exception: # noqa: BLE001 — observers must not break mutations logger.warning( - "[adcp.buyer_agent_registry] mutation observer raised for " - "op=%s agent_url=%s", + "[adcp.buyer_agent_registry] mutation observer raised for op=%s agent_url=%s", op, agent_url, exc_info=True, @@ -514,8 +538,17 @@ def _sync_lookup_by_agent_url(self, agent_url: str) -> BuyerAgent | None: def _sync_lookup_by_api_key_id(self, key: str) -> BuyerAgent | None: with self._pool.connection() as conn, conn.cursor() as cur: cur.execute(self._sql_select_by_api_key_id, (key,)) - row = cur.fetchone() - return _row_to_agent(row) if row else None + rows = cur.fetchall() + if len(rows) > 1: + # Defense in depth for deployments that have not yet applied + # the unique-index migration. Never select an arbitrary + # commercial identity for an ambiguous credential. + logger.error( + "PgBuyerAgentRegistry rejected an ambiguous credential mapping; " + "apply the unique api_key_id index migration" + ) + return None + return _row_to_agent(rows[0]) if rows else None def _row_to_agent(row: Any) -> BuyerAgent: diff --git a/src/adcp/decisioning/pg/buyer_agent_registry.sql b/src/adcp/decisioning/pg/buyer_agent_registry.sql index db8a4f54d..519b9b803 100644 --- a/src/adcp/decisioning/pg/buyer_agent_registry.sql +++ b/src/adcp/decisioning/pg/buyer_agent_registry.sql @@ -57,13 +57,28 @@ CREATE TABLE IF NOT EXISTS adcp_buyer_agents ( updated_at TIMESTAMPTZ NOT NULL DEFAULT now() ); --- Bearer-credential lookup index. Partial — only rows with an --- api_key_id pay the index cost. Signing-only adopters store no --- bearer keys and the index stays empty. -CREATE INDEX IF NOT EXISTS adcp_buyer_agents_api_key_id_idx +-- A credential identifier must resolve to exactly one commercial identity. +-- Partial — signing-only adopters store NULL and do not occupy the index. +-- Existing deployments should resolve any duplicates before applying this +-- migration; index creation intentionally fails closed when duplicates exist. +DO $$ +BEGIN + IF EXISTS ( + SELECT 1 FROM adcp_buyer_agents + WHERE api_key_id IS NOT NULL + GROUP BY api_key_id HAVING COUNT(*) > 1 + ) THEN + RAISE EXCEPTION + 'Cannot enforce adcp_buyer_agents.api_key_id uniqueness: rotate or remove duplicate bearer credentials, then rerun'; + END IF; +END $$; + +CREATE UNIQUE INDEX IF NOT EXISTS adcp_buyer_agents_api_key_id_uidx ON adcp_buyer_agents (api_key_id) WHERE api_key_id IS NOT NULL; +DROP INDEX IF EXISTS adcp_buyer_agents_api_key_id_idx; + -- Suspension / blocking sweep helper — admin tools that list -- agents-needing-attention can scan by status efficiently without -- a sequential scan. diff --git a/src/adcp/decisioning/pg/proposal_store.py b/src/adcp/decisioning/pg/proposal_store.py index 427a0b474..9b8d021bb 100644 --- a/src/adcp/decisioning/pg/proposal_store.py +++ b/src/adcp/decisioning/pg/proposal_store.py @@ -554,30 +554,15 @@ async def get( self, proposal_id: str, *, - expected_account_id: str | None = None, + expected_account_id: str, ) -> ProposalRecord | None: - # The Protocol allows expected_account_id=None — historically a - # convenience for diagnostic / admin callers. We still serve - # that case but route it through a separate query without the - # account predicate so the tenancy-aware fast path is purely - # parameterised; a future code reader can't accidentally pass - # None into a tenancy-required call. - if expected_account_id is None: - sql = ( # noqa: S608 — table name pre-validated at construction - f"SELECT proposal_id, account_id, state, recipes, " - f"proposal_payload, expires_at, media_buy_id, " - f"recipe_schema_version FROM {self._table} " - f"WHERE proposal_id = %s" - ) - params: tuple[Any, ...] = (proposal_id,) - else: - sql = ( # noqa: S608 - f"SELECT proposal_id, account_id, state, recipes, " - f"proposal_payload, expires_at, media_buy_id, " - f"recipe_schema_version FROM {self._table} " - f"WHERE account_id = %s AND proposal_id = %s" - ) - params = (expected_account_id, proposal_id) + sql = ( # noqa: S608 — table name pre-validated at construction + f"SELECT proposal_id, account_id, state, recipes, " + f"proposal_payload, expires_at, media_buy_id, " + f"recipe_schema_version FROM {self._table} " + f"WHERE account_id = %s AND proposal_id = %s" + ) + params: tuple[Any, ...] = (expected_account_id, proposal_id) async with self._pool.connection() as conn: cur = await conn.execute(sql, params) row = await cur.fetchone() diff --git a/src/adcp/decisioning/platform.py b/src/adcp/decisioning/platform.py index 295b36d05..6c71ed71f 100644 --- a/src/adcp/decisioning/platform.py +++ b/src/adcp/decisioning/platform.py @@ -21,6 +21,7 @@ from adcp.decisioning.upstream import ( NoAuth, UpstreamAuth, + UpstreamClientPool, UpstreamHttpClient, ) from adcp.types.capabilities import ( @@ -49,6 +50,10 @@ from adcp.types import GetAdcpCapabilitiesRequest +_NO_AUTH = NoAuth() +_DEFAULT_UPSTREAM_CLIENT_CACHE_SIZE = 128 + + @dataclass class DecisioningCapabilities: """What a platform claims to support. @@ -421,6 +426,12 @@ def create_media_buy(self, req, ctx): #: this attribute. upstream_url: str | None = None + #: Maximum per-platform upstream client pools retained by + #: :meth:`upstream_for`. Least-recently-used entries are closed on + #: eviction. Increase only when one platform intentionally uses more + #: than 128 stable URL/auth/header/transport combinations. + upstream_client_cache_size: int = _DEFAULT_UPSTREAM_CLIENT_CACHE_SIZE + def get_adcp_capabilities_for_request( self, params: GetAdcpCapabilitiesRequest | dict[str, Any] | None = None, @@ -470,11 +481,11 @@ def upstream_for( the client at the per-tenant fixture URL. Adapter business logic runs unchanged. - Clients are cached per-platform-instance keyed by - ``(base_url, id(auth))`` so repeated requests pool connections - through one ``httpx.AsyncClient``. Different auth strategies - get distinct clients (the auth is injected at construction - and can't be swapped per-request from a cached client). + Clients are pooled per platform with a bounded LRU keyed by URL, + auth identity, headers, timeout, and 404 behavior. Different auth + strategies get distinct clients. Eviction retires a client without + closing it under an in-flight borrower; framework shutdown closes + both cached and retired clients. :param ctx: The current request context. Required for ``ctx.account.mode`` and ``ctx.account.metadata``. @@ -540,7 +551,7 @@ def upstream_for( return self._cached_upstream_client( base_url=base_url, - auth=auth or NoAuth(), + auth=auth or _NO_AUTH, default_headers=default_headers, timeout=timeout, treat_404_as_none=treat_404_as_none, @@ -557,35 +568,36 @@ def _cached_upstream_client( ) -> UpstreamHttpClient: """Per-instance cached :class:`UpstreamHttpClient` factory. - Cache key is ``(base_url, id(auth))``. Pooling correctness + Pool identity includes URL, auth identity, default headers, and + transport behavior. Pooling correctness requires keying on the auth instance — different ``DynamicBearer`` closures for different tenants need distinct clients so the token resolver doesn't get accidentally shared, and the ``UpstreamHttpClient`` itself owns the underlying ``httpx.AsyncClient`` connection pool. - Cache lives on the platform instance (``__dict__`` lazy init); - multi-platform processes don't cross-pollute. Adopter code - does not mutate the cache; lifecycle is "create once, reuse - for the platform instance's lifetime." + The owning :class:`UpstreamClientPool` lives on the platform instance + and bounds its hot LRU. Evicted clients are retired, not closed under + active borrowers, and all owned clients drain at framework shutdown + (or an explicit :meth:`aclose_upstream_clients` call). """ - cache: dict[tuple[str, int], UpstreamHttpClient] | None - cache = getattr(self, "_upstream_client_cache", None) - if cache is None: - cache = {} - self._upstream_client_cache = cache - - key = (base_url, id(auth)) - existing = cache.get(key) - if existing is not None: - return existing - - client = UpstreamHttpClient( + pool: UpstreamClientPool | None = getattr(self, "_upstream_client_pool", None) + if pool is None: + max_size = int( + getattr(self, "upstream_client_cache_size", _DEFAULT_UPSTREAM_CLIENT_CACHE_SIZE) + ) + pool = UpstreamClientPool(max_size=max_size) + self._upstream_client_pool = pool + return pool.get( base_url=base_url, auth=auth, default_headers=default_headers, timeout=timeout, treat_404_as_none=treat_404_as_none, ) - cache[key] = client - return client + + async def aclose_upstream_clients(self) -> None: + """Release all clients owned by this platform's upstream pool.""" + pool: UpstreamClientPool | None = getattr(self, "_upstream_client_pool", None) + if pool is not None: + await pool.aclose() diff --git a/src/adcp/decisioning/proposal_store.py b/src/adcp/decisioning/proposal_store.py index a1ca68a69..89b791d73 100644 --- a/src/adcp/decisioning/proposal_store.py +++ b/src/adcp/decisioning/proposal_store.py @@ -187,16 +187,14 @@ def get( self, proposal_id: str, *, - expected_account_id: str | None = None, + expected_account_id: str, ) -> MaybeAsync[ProposalRecord | None]: - """Look up a proposal record. Cross-tenant probes return ``None``. - - Mirrors :meth:`adcp.decisioning.TaskRegistry.get`'s posture: - when ``expected_account_id`` is supplied, a mismatch returns - ``None`` rather than the raw record. The dispatch path always - passes the authenticated principal's account_id; adopter - impls MUST honor this — returning a cross-tenant record - enables principal-enumeration via proposal_id probing. + """Look up a proposal record within one authenticated account scope. + + ``expected_account_id`` is mandatory: proposal ids are only unique + within an account and an unscoped lookup can disclose another tenant's + record or make one tenant's result depend on another tenant's ids. A + mismatch returns ``None`` rather than a raw record. """ ... @@ -407,7 +405,7 @@ def __init__( ``lambda: datetime.now(timezone.utc)``. Tests pin a deterministic clock to validate eviction. """ - self._records: dict[str, ProposalRecord] = {} + self._records: dict[tuple[str, str], ProposalRecord] = {} # Reverse index keyed by (account_id, media_buy_id). Tenant scoping # in the key prevents collisions when adopter media_buy_ids overlap # across tenants (sequential IDs, deterministic test fixtures, etc.) — @@ -417,24 +415,24 @@ def __init__( self._draft_ttl = draft_ttl self._committed_grace = committed_grace self._clock = clock or (lambda: datetime.now(timezone.utc)) - self._creation_times: dict[str, datetime] = {} + self._creation_times: dict[tuple[str, str], datetime] = {} def _evict_expired_locked(self) -> None: """Remove records past their TTL. Must be called under the lock.""" now = self._clock() - to_remove: list[str] = [] - for proposal_id, record in self._records.items(): - created = self._creation_times.get(proposal_id, now) + to_remove: list[tuple[str, str]] = [] + for key, record in self._records.items(): + created = self._creation_times.get(key, now) if record.state == ProposalState.DRAFT: if now - created > self._draft_ttl: - to_remove.append(proposal_id) + to_remove.append(key) elif record.expires_at is not None: deadline = record.expires_at + self._committed_grace if now > deadline: - to_remove.append(proposal_id) - for proposal_id in to_remove: - removed = self._records.pop(proposal_id, None) - self._creation_times.pop(proposal_id, None) + to_remove.append(key) + for key in to_remove: + removed = self._records.pop(key, None) + self._creation_times.pop(key, None) if removed is not None and removed.media_buy_id is not None: self._media_buy_index.pop((removed.account_id, removed.media_buy_id), None) @@ -448,7 +446,8 @@ async def put_draft( ) -> None: async with self._lock: self._evict_expired_locked() - existing = self._records.get(proposal_id) + key = (account_id, proposal_id) + existing = self._records.get(key) if existing is not None and existing.state != ProposalState.DRAFT: raise AdcpError( "INTERNAL_ERROR", @@ -467,29 +466,23 @@ async def put_draft( recipes=dict(recipes), proposal_payload=dict(proposal_payload), ) - self._records[proposal_id] = record + self._records[key] = record # Track creation time only for fresh records — refine # iterations preserve the original creation time so the # 24h draft TTL is anchored to the start of the buyer's # session, not the most recent iteration. - if proposal_id not in self._creation_times: - self._creation_times[proposal_id] = self._clock() + if key not in self._creation_times: + self._creation_times[key] = self._clock() async def get( self, proposal_id: str, *, - expected_account_id: str | None = None, + expected_account_id: str, ) -> ProposalRecord | None: async with self._lock: self._evict_expired_locked() - record = self._records.get(proposal_id) - if record is None: - return None - if expected_account_id is not None and record.account_id != expected_account_id: - # Cross-tenant probe — return None, not raw record. - return None - return record + return self._records.get((expected_account_id, proposal_id)) async def commit( self, @@ -501,8 +494,9 @@ async def commit( ) -> None: async with self._lock: self._evict_expired_locked() - record = self._records.get(proposal_id) - if record is None or record.account_id != expected_account_id: + key = (expected_account_id, proposal_id) + record = self._records.get(key) + if record is None: # Cross-tenant probe collapses to "not in store" — same # principal-enumeration defence as :meth:`get`. raise AdcpError( @@ -539,7 +533,7 @@ async def commit( ), recovery="terminal", ) - self._records[proposal_id] = replace( + self._records[key] = replace( record, state=ProposalState.COMMITTED, expires_at=expires_at, @@ -554,10 +548,11 @@ async def try_reserve_consumption( ) -> ProposalRecord: async with self._lock: self._evict_expired_locked() - record = self._records.get(proposal_id) + key = (expected_account_id, proposal_id) + record = self._records.get(key) # Cross-tenant probe collapses to PROPOSAL_NOT_FOUND — same # principal-enumeration defense as :meth:`get`. - if record is None or record.account_id != expected_account_id: + if record is None: raise AdcpError( "PROPOSAL_NOT_FOUND", message=(f"Proposal {proposal_id!r} not found."), @@ -577,7 +572,7 @@ async def try_reserve_consumption( field="proposal_id", ) reserved = replace(record, state=ProposalState.CONSUMING) - self._records[proposal_id] = reserved + self._records[key] = reserved return reserved async def finalize_consumption( @@ -588,8 +583,9 @@ async def finalize_consumption( expected_account_id: str, ) -> None: async with self._lock: - record = self._records.get(proposal_id) - if record is None or record.account_id != expected_account_id: + key = (expected_account_id, proposal_id) + record = self._records.get(key) + if record is None: raise AdcpError( "INTERNAL_ERROR", message=( @@ -622,7 +618,7 @@ async def finalize_consumption( ), recovery="terminal", ) - self._records[proposal_id] = replace( + self._records[key] = replace( record, state=ProposalState.CONSUMED, media_buy_id=media_buy_id, @@ -636,8 +632,9 @@ async def release_consumption( expected_account_id: str, ) -> None: async with self._lock: - record = self._records.get(proposal_id) - if record is None or record.account_id != expected_account_id: + key = (expected_account_id, proposal_id) + record = self._records.get(key) + if record is None: # Idempotent — releasing an unknown id is a no-op so the # adapter-failure rollback path can be unconditional. return @@ -654,7 +651,7 @@ async def release_consumption( ), recovery="terminal", ) - self._records[proposal_id] = replace( + self._records[key] = replace( record, state=ProposalState.COMMITTED, ) @@ -671,8 +668,9 @@ async def mark_consumed( # two-phase methods directly. async with self._lock: self._evict_expired_locked() - record = self._records.get(proposal_id) - if record is None or record.account_id != expected_account_id: + key = (expected_account_id, proposal_id) + record = self._records.get(key) + if record is None: raise AdcpError( "INTERNAL_ERROR", message=( @@ -704,7 +702,7 @@ async def mark_consumed( ), recovery="terminal", ) - self._records[proposal_id] = replace( + self._records[key] = replace( record, state=ProposalState.CONSUMED, media_buy_id=media_buy_id, @@ -718,12 +716,13 @@ async def discard( expected_account_id: str, ) -> None: async with self._lock: - record = self._records.get(proposal_id) - if record is None or record.account_id != expected_account_id: + key = (expected_account_id, proposal_id) + record = self._records.get(key) + if record is None: # Idempotent — unknown id or cross-tenant probe is a no-op. return - self._records.pop(proposal_id, None) - self._creation_times.pop(proposal_id, None) + self._records.pop(key, None) + self._creation_times.pop(key, None) if record.media_buy_id is not None: self._media_buy_index.pop((record.account_id, record.media_buy_id), None) @@ -738,7 +737,7 @@ async def get_by_media_buy_id( proposal_id = self._media_buy_index.get((expected_account_id, media_buy_id)) if proposal_id is None: return None - record = self._records.get(proposal_id) + record = self._records.get((expected_account_id, proposal_id)) if record is None: # Index drift — clean up. self._media_buy_index.pop((expected_account_id, media_buy_id), None) diff --git a/src/adcp/decisioning/registry_cache.py b/src/adcp/decisioning/registry_cache.py index 76b1b7e3a..1f95f2ec4 100644 --- a/src/adcp/decisioning/registry_cache.py +++ b/src/adcp/decisioning/registry_cache.py @@ -12,8 +12,8 @@ an enumeration probe walking a million ``agent_url`` strings would otherwise hit the DB once per probe; with negative caching it hits the DB once per ``(tenant, agent_url)`` pair within the TTL window. -* :class:`RateLimitedBuyerAgentRegistry` — per-(tenant, lookup-key) - token bucket. On exhaustion, raises ``PERMISSION_DENIED`` with no +* :class:`RateLimitedBuyerAgentRegistry` — aggregate per-tenant plus + per-(tenant, lookup-key) token buckets. On exhaustion, raises ``PERMISSION_DENIED`` with no ``details`` so the wire shape matches every other denied path (registry miss, suspended, blocked) — preserves the spec's omit-on-unestablished-identity rule from PR #393. A distinct @@ -457,8 +457,17 @@ class _Bucket: last_refill: float +@dataclass +class _TenantBuckets: + """Rate-limit state owned by one tenant boundary.""" + + aggregate: _Bucket + lookups: OrderedDict[str, _Bucket] + last_seen: float + + class RateLimitedBuyerAgentRegistry: - """Per-tenant token-bucket rate limiter wrapping a + """Aggregate per-tenant and per-lookup token-bucket rate limiter wrapping a :class:`BuyerAgentRegistry`. Sized for the credential-stuffing oracle: the registry's @@ -469,14 +478,19 @@ class RateLimitedBuyerAgentRegistry: the SQL query runs. :param inner: The wrapped :class:`BuyerAgentRegistry`. - :param rps_per_tenant: Steady-state requests per second per - ``(tenant_id, lookup_key)`` bucket. Default 100 — high - enough to absorb a real buyer's storyboard burst, low - enough that an enumeration probe at line rate gets cut off. + :param rps_per_tenant: Steady-state requests per second for the + tenant aggregate. Default 100 — high enough to absorb a real + buyer's storyboard burst, low enough that an enumeration probe + at line rate gets cut off. :param burst: Maximum bucket capacity (tokens). Default ``rps_per_tenant`` so a steady state can sustain ``rps_per_tenant`` calls/sec but bursts are capped at the same number. Adopters with bursty real traffic raise this. + :param rps_per_lookup: Optional independent steady-state limit for + each lookup identity. Disabled by default because a lookup tier + with the same rate and burst as the aggregate can never bind. + :param lookup_burst: Maximum per-lookup bucket capacity. Requires + ``rps_per_lookup`` and defaults to that rate. :param audit_sink: Optional audit sink — emits ``rate_limited`` events when the bucket is exhausted. The most interesting event for security review (repeated rate-limit exhaustion @@ -484,6 +498,12 @@ class RateLimitedBuyerAgentRegistry: probing). :param time_source: Override for tests — defaults to :func:`time.monotonic`. + :param max_buckets: Hard cap per tenant across its aggregate and + per-lookup bucket state. When full, that tenant's least-recently-used + lookup bucket is replaced; the aggregate budget still bounds rotating + probes and another tenant's allocation is never consumed. + :param bucket_idle_ttl_seconds: Idle bucket retention. Discarding an idle + bucket is safe because it would have refilled to its full burst. Failure mode ------------ @@ -503,28 +523,71 @@ def __init__( *, rps_per_tenant: float = 100.0, burst: float | None = None, + rps_per_lookup: float | None = None, + lookup_burst: float | None = None, audit_sink: AuditSink | None = None, sink_timeout_seconds: float = 5.0, time_source: Callable[[], float] = time.monotonic, + max_buckets: int = 10_000, + bucket_idle_ttl_seconds: float = 300.0, ) -> None: if rps_per_tenant <= 0: raise ValueError(f"rps_per_tenant must be > 0, got {rps_per_tenant!r}") if burst is not None and burst <= 0: raise ValueError(f"burst must be > 0, got {burst!r}") + if rps_per_lookup is not None and rps_per_lookup <= 0: + raise ValueError(f"rps_per_lookup must be > 0, got {rps_per_lookup!r}") + if lookup_burst is not None and lookup_burst <= 0: + raise ValueError(f"lookup_burst must be > 0, got {lookup_burst!r}") + if lookup_burst is not None and rps_per_lookup is None: + raise ValueError("lookup_burst requires rps_per_lookup") + if max_buckets < 2: + raise ValueError(f"max_buckets must be >= 2, got {max_buckets!r}") + if bucket_idle_ttl_seconds <= 0: + raise ValueError( + f"bucket_idle_ttl_seconds must be > 0, got {bucket_idle_ttl_seconds!r}" + ) + tenant_burst = burst if burst is not None else rps_per_tenant + per_lookup_burst = ( + lookup_burst + if lookup_burst is not None + else (rps_per_lookup if rps_per_lookup is not None else None) + ) + if bucket_idle_ttl_seconds < tenant_burst / rps_per_tenant: + raise ValueError( + "bucket_idle_ttl_seconds must be >= burst / rps_per_tenant " + "so idle eviction cannot reset a partially refilled budget" + ) + if ( + rps_per_lookup is not None + and per_lookup_burst is not None + and bucket_idle_ttl_seconds < per_lookup_burst / rps_per_lookup + ): + raise ValueError( + "bucket_idle_ttl_seconds must be >= lookup_burst / rps_per_lookup " + "so idle eviction cannot reset a partially refilled budget" + ) self._inner = inner self._rate = rps_per_tenant - self._burst = burst if burst is not None else rps_per_tenant + self._burst = tenant_burst + self._lookup_rate = rps_per_lookup + self._lookup_burst = per_lookup_burst self._sink = audit_sink self._sink_timeout = sink_timeout_seconds self._now = time_source - self._buckets: dict[tuple[str | None, str], _Bucket] = {} + self._max_buckets = max_buckets + self._bucket_idle_ttl = bucket_idle_ttl_seconds + # Tenant partitioning is the isolation boundary. ``max_buckets`` + # applies inside each value, so one tenant can never crowd another + # tenant's aggregate bucket out of a process-global namespace. + self._buckets: OrderedDict[str | None, _TenantBuckets] = OrderedDict() self._lock = asyncio.Lock() async def resolve_by_agent_url(self, agent_url: str) -> BuyerAgent | None: tenant_id = _current_tenant_id() lookup_key = f"agent_url:{agent_url}" await self._charge( - (tenant_id, lookup_key), + lookup_key, operation="buyer_agent_registry.resolve_by_agent_url", tenant_id=tenant_id, ) @@ -534,7 +597,7 @@ async def resolve_by_credential(self, credential: Credential) -> BuyerAgent | No tenant_id = _current_tenant_id() lookup_key = _credential_key(credential) await self._charge( - (tenant_id, lookup_key), + lookup_key, operation="buyer_agent_registry.resolve_by_credential", tenant_id=tenant_id, ) @@ -542,7 +605,7 @@ async def resolve_by_credential(self, credential: Credential) -> BuyerAgent | No async def _charge( self, - key: tuple[str | None, str], + lookup_key: str, *, operation: str, tenant_id: str | None, @@ -551,34 +614,102 @@ async def _charge( exhaustion.""" now = self._now() async with self._lock: - bucket = self._buckets.get(key) - if bucket is None: - # New bucket — start full so a fresh tenant gets the - # burst allowance immediately. - bucket = _Bucket(tokens=self._burst, last_refill=now) - self._buckets[key] = bucket - else: - # Refill at ``rate`` tokens/sec, capped at ``burst``. - elapsed = now - bucket.last_refill - bucket.tokens = min(self._burst, bucket.tokens + elapsed * self._rate) - bucket.last_refill = now - if bucket.tokens < 1.0: - exhausted = True + self._prune_idle(now) + tenant_buckets = self._buckets.get(tenant_id) + if tenant_buckets is None: + tenant_buckets = _TenantBuckets( + aggregate=_Bucket(tokens=self._burst, last_refill=now), + lookups=OrderedDict(), + last_seen=now, + ) + self._buckets[tenant_id] = tenant_buckets else: - bucket.tokens -= 1.0 - exhausted = False + tenant_buckets.last_seen = now + self._buckets.move_to_end(tenant_id) + + # Check the narrower lookup budget first. Once a hot key is + # exhausted, repeated probes must not drain the shared tenant + # aggregate and deny unrelated identities in that tenant. + exhausted = False + if self._lookup_rate is not None and self._lookup_burst is not None: + self._prune_idle_lookups(tenant_buckets, now) + exhausted = not self._spend_lookup_locked(tenant_buckets, lookup_key, now) + if not exhausted: + exhausted = not self._spend_bucket( + tenant_buckets.aggregate, + now, + rate=self._rate, + burst=self._burst, + ) if exhausted: # Audit emission OUTSIDE the lock — the sink may be slow. await _emit_audit( self._sink, operation=operation, outcome="rate_limited", - lookup_key=key[1], + lookup_key=lookup_key, tenant_id=tenant_id, sink_timeout_seconds=self._sink_timeout, ) raise _denied_error() + def _spend_lookup_locked( + self, + tenant_buckets: _TenantBuckets, + lookup_key: str, + now: float, + ) -> bool: + bucket = tenant_buckets.lookups.get(lookup_key) + if bucket is None: + # The aggregate consumes one slot from the per-tenant cap. + if len(tenant_buckets.lookups) >= self._max_buckets - 1: + tenant_buckets.lookups.popitem(last=False) + assert self._lookup_burst is not None + bucket = _Bucket(tokens=self._lookup_burst, last_refill=now) + tenant_buckets.lookups[lookup_key] = bucket + else: + tenant_buckets.lookups.move_to_end(lookup_key) + assert self._lookup_rate is not None + assert self._lookup_burst is not None + return self._spend_bucket( + bucket, + now, + rate=self._lookup_rate, + burst=self._lookup_burst, + ) + + @staticmethod + def _spend_bucket( + bucket: _Bucket, + now: float, + *, + rate: float, + burst: float, + ) -> bool: + elapsed = max(0.0, now - bucket.last_refill) + bucket.tokens = min(burst, bucket.tokens + elapsed * rate) + bucket.last_refill = now + if bucket.tokens < 1.0: + return False + bucket.tokens -= 1.0 + return True + + def _prune_idle(self, now: float) -> None: + """Discard up to 16 safely idle tenant partitions.""" + for _ in range(min(16, len(self._buckets))): + tenant_id, tenant_buckets = next(iter(self._buckets.items())) + if now - tenant_buckets.last_seen < self._bucket_idle_ttl: + return + self._buckets.pop(tenant_id) + + def _prune_idle_lookups(self, tenant_buckets: _TenantBuckets, now: float) -> None: + """Reclaim safely refilled lookup slots within an active tenant.""" + for _ in range(min(16, len(tenant_buckets.lookups))): + lookup_key, bucket = next(iter(tenant_buckets.lookups.items())) + if now - bucket.last_refill < self._bucket_idle_ttl: + return + tenant_buckets.lookups.pop(lookup_key) + # ----- Audit-emitting terminal wrapper ----------------------------- diff --git a/src/adcp/decisioning/roster_store.py b/src/adcp/decisioning/roster_store.py index 1f5d42a94..715ff9d1d 100644 --- a/src/adcp/decisioning/roster_store.py +++ b/src/adcp/decisioning/roster_store.py @@ -25,10 +25,9 @@ Design notes ------------ -* **Roster IS the allowlist.** Auth-based filtering happens upstream of - this layer — the framework's account-resolution gate enforces - principal-vs-account scope. The store does not consult ``ctx`` to - filter ``list``. +* **Roster membership is not authorization.** A caller-supplied + ``authorize`` callback binds verified auth to each account. It is a + required factory argument so missing policy fails visibly at boot. * **Immutable post-construction.** The input dict is copied into an internal :class:`MappingProxyType` so external mutation of the caller's dict cannot widen the allowlist after the fact. Adopters @@ -53,15 +52,24 @@ store = create_roster_account_store( roster={ - "acct_alpha": Account(id="acct_alpha", name="Alpha", status="active"), - "acct_beta": Account(id="acct_beta", name="Beta", status="active"), + "acct_alpha": Account( + id="acct_alpha", name="Alpha", status="active", + metadata={"principals": {"https://buyer-alpha.example/"}}, + ), + "acct_beta": Account( + id="acct_beta", name="Beta", status="active", + metadata={"principals": {"https://buyer-beta.example/"}}, + ), }, + authorize=lambda account, auth: auth.principal in account.metadata["principals"], ) """ from __future__ import annotations -from collections.abc import Mapping +import inspect +import logging +from collections.abc import Awaitable, Callable, Mapping from types import MappingProxyType from typing import TYPE_CHECKING, Any, Generic, Literal @@ -82,6 +90,8 @@ __all__ = ["create_roster_account_store"] +logger = logging.getLogger(__name__) + #: Per-platform metadata generic. Defaults to ``dict[str, Any]`` for #: adopters who don't define a typed metadata shape. TMeta = TypeVar("TMeta", default=dict[str, Any]) @@ -106,7 +116,11 @@ class _RosterAccountStore(Generic[TMeta]): resolution: Literal["explicit"] = "explicit" - def __init__(self, roster: Mapping[str, Account[TMeta]]) -> None: + def __init__( + self, + roster: Mapping[str, Account[TMeta]], + authorize: Callable[[Account[TMeta], AuthInfo], bool | Awaitable[bool]], + ) -> None: # Copy into a plain dict, then wrap in MappingProxyType so the # store's view is decoupled from the caller's input. Two layers # of protection: external mutation of the input dict can't @@ -121,6 +135,21 @@ def __init__(self, roster: Mapping[str, Account[TMeta]]) -> None: f"its key" ) self._roster: Mapping[str, Account[TMeta]] = MappingProxyType(copied) + self._authorize = authorize + + async def _is_authorized(self, account: Account[TMeta], auth_info: AuthInfo | None) -> bool: + if auth_info is None: + return False + try: + result = self._authorize(account, auth_info) + if inspect.isawaitable(result): + result = await result + return result is True + except Exception: + # Authorization callbacks are security controls: callback + # failures deny access and never widen the roster. + logger.exception("roster authorize callback raised; denying") + return False async def resolve( self, @@ -136,14 +165,17 @@ async def resolve( Signature mirrors the :class:`AccountStore` Protocol's ``resolve(ref, auth_info=None)`` — the framework dispatcher passes ``auth_info`` as a keyword argument. ``auth_info`` is - accepted for Protocol parity but unused: the roster IS the - allowlist, no auth-based filtering at this layer. + required for access. Missing auth, callback failure, and callback + denial all collapse to ``None`` so a foreign id is + indistinguishable from an unknown id. """ - del auth_info # roster is the allowlist; no per-principal filtering account_id = ref_account_id(ref) if account_id is None: return None - return self._roster.get(account_id) + account = self._roster.get(account_id) + if account is None or not await self._is_authorized(account, auth_info): + return None + return account async def upsert( self, @@ -222,17 +254,18 @@ async def list( filter: dict[str, Any] | None = None, ctx: ResolveContext | None = None, ) -> list[Account[TMeta]]: - """Return every roster entry. - - Adopters who need filtering (status, sandbox, pagination) wrap - ``list`` and post-filter the returned list — the roster store - does not interpret ``filter`` because the typical roster - cardinality (single-digit to low-thousands of accounts per - publisher) is small enough that in-memory filtering at the - adopter layer is fine. + """Return roster entries authorized for the verified principal. + + Missing auth returns ``[]``. + The optional wire ``filter`` remains adopter-defined. """ - del filter, ctx - return list(self._roster.values()) + del filter + auth_info = ctx.auth_info if ctx is not None else None + return [ + account + for account in self._roster.values() + if await self._is_authorized(account, auth_info) + ] def _ref_brand(ref: AccountReference | None) -> dict[str, Any]: @@ -267,10 +300,16 @@ def _ref_operator(ref: AccountReference | None) -> str: def create_roster_account_store( *, roster: Mapping[str, Account[TMeta]], + authorize: Callable[[Account[TMeta], AuthInfo], bool | Awaitable[bool]], ) -> _RosterAccountStore[TMeta]: """Build an :class:`AccountStore` backed by a fixed publisher- curated roster. + ``authorize`` is the required security boundary binding a verified + principal to an account. It may be synchronous or asynchronous. + The callback is required so a roster cannot be mistaken for an + authorization policy. Possession of an account id never grants access. + The returned object conforms to the :class:`AccountStore` Protocol plus the optional :class:`AccountStoreList`, :class:`AccountStoreUpsert`, and :class:`AccountStoreSyncGovernance` @@ -284,12 +323,16 @@ def create_roster_account_store( :class:`ValueError` at construction. The mapping is copied into an internal immutable view, so subsequent mutation of the caller's dict does not affect the store. + :param authorize: Principal/account authorization callback. Receives + the candidate account and verified auth info. Return exactly + ``True`` to grant access. May be async. ``False`` or an exception + denies access. :returns: An :class:`AccountStore` whose: - * :meth:`resolve` returns the roster entry for an + * :meth:`resolve` returns an authorized roster entry for an ``account_id``-arm ref, ``None`` otherwise. - * :meth:`list` returns every roster entry. + * :meth:`list` returns only authorized roster entries. * :meth:`upsert` rejects every input entry with ``PERMISSION_DENIED``. * :meth:`sync_governance` rejects every input entry with @@ -298,4 +341,4 @@ def create_roster_account_store( :raises ValueError: When any roster value's ``id`` does not match its dict key. """ - return _RosterAccountStore(roster) + return _RosterAccountStore(roster, authorize) diff --git a/src/adcp/decisioning/serve.py b/src/adcp/decisioning/serve.py index 05e4ac2c9..139b477d3 100644 --- a/src/adcp/decisioning/serve.py +++ b/src/adcp/decisioning/serve.py @@ -32,6 +32,7 @@ import os import warnings from concurrent.futures import ThreadPoolExecutor +from dataclasses import replace from typing import TYPE_CHECKING, Any from adcp.decisioning.dispatch import validate_platform @@ -638,6 +639,26 @@ def serve( debug_traffic_source = mock_ad_server.get_traffic if mock_ad_server is not None else None if pre_validation_hooks is not None: serve_kwargs["pre_validation_hooks"] = pre_validation_hooks + + # The SDK owns the parent lifespan for the dual-transport server, so + # attach the platform's upstream pool drain there. Single-transport + # servers do not yet expose lifecycle hooks; those adopters retain the + # explicit ``platform.aclose_upstream_clients()`` seam. + config = serve_kwargs.get("config") + effective_transport = ( + config.transport if config is not None else serve_kwargs.get("transport", "streamable-http") + ) + if effective_transport == "both": + if config is not None: + serve_kwargs["config"] = replace( + config, + on_shutdown=(*tuple(config.on_shutdown or ()), platform.aclose_upstream_clients), + ) + else: + serve_kwargs["on_shutdown"] = ( + *tuple(serve_kwargs.get("on_shutdown") or ()), + platform.aclose_upstream_clients, + ) _adcp_serve( handler, name=server_name, diff --git a/src/adcp/decisioning/tenant_store.py b/src/adcp/decisioning/tenant_store.py index c346080ce..504249e86 100644 --- a/src/adcp/decisioning/tenant_store.py +++ b/src/adcp/decisioning/tenant_store.py @@ -4,8 +4,8 @@ Solves the recurring class of bug where adopters routing by wire-supplied operator without cross-checking the auth principal could write across tenants. The gate is enforced inside the framework: cross-tenant entries -on ``upsert`` / ``sync_governance`` are rejected with -``PERMISSION_DENIED`` before reaching adopter callbacks. +on ``upsert`` / ``sync_governance`` collapse to ``ACCOUNT_NOT_FOUND`` +before reaching adopter callbacks. Mirrors the JS-side ``createTenantStore`` at ``packages/sdk/src/server/decisioning/tenant-store.ts`` (6.7). The @@ -145,7 +145,9 @@ def _permission_denied_message(ref: Any) -> str: ) -def _build_failed_sync_accounts_row(ref: Any, code: str, message: str) -> SyncAccountsResultRow: +def _build_failed_sync_accounts_row( + ref: Any, code: str, message: str, recovery: str +) -> SyncAccountsResultRow: """Construct a wire-shaped failure row for ``sync_accounts``. The wire schema requires ``brand`` + ``operator`` on every row, so @@ -167,18 +169,18 @@ def _build_failed_sync_accounts_row(ref: Any, code: str, message: str) -> SyncAc operator=operator, action="failed", status="rejected", - errors=[{"code": code, "message": message}], + errors=[{"code": code, "message": message, "recovery": recovery}], account_id=account_id if isinstance(account_id, str) else None, ) def _build_failed_sync_governance_row( - entry: SyncGovernanceEntry, code: str, message: str + entry: SyncGovernanceEntry, code: str, message: str, recovery: str ) -> SyncGovernanceResultRow: return SyncGovernanceResultRow( account=entry.account, status="failed", - errors=[{"code": code, "message": message}], + errors=[{"code": code, "message": message, "recovery": recovery}], ) @@ -251,6 +253,55 @@ async def _auth_tenant(self, ctx: ResolveContext) -> str | None: """Compute the auth principal's tenant once per request.""" return cast("str | None", await _await_maybe(self._resolve_from_auth(ctx))) + async def _classify_entry_access( + self, + ref: Any, + ctx: ResolveContext, + auth_tid: str | None, + *, + operation: str, + ) -> tuple[str, str, str] | None: + """Return a public failure for one account ref, or ``None`` if allowed. + + Unknown and cross-tenant references intentionally share the exact + ``ACCOUNT_NOT_FOUND`` construction. This is the AdCP 3.1 + ``sync-governance-response`` existence-hiding posture: a caller must + not learn whether a reference exists outside its tenant. Missing auth + and callback failures remain ``PERMISSION_DENIED`` fail-closed results. + """ + try: + entry_account = cast( + "Account[TMeta] | None", + await _await_maybe(self._resolve_by_ref(ref, ctx)), + ) + except Exception: + logger.warning( + "tenant_store.%s: resolve_by_ref raised for entry; " + "rejecting with PERMISSION_DENIED", + operation, + exc_info=True, + ) + return "PERMISSION_DENIED", _permission_denied_message(ref), "correctable" + + if entry_account is None: + return "ACCOUNT_NOT_FOUND", _account_not_found_message(ref), "terminal" + + try: + entry_tid = self._tenant_id(entry_account) + except Exception: + logger.warning( + "tenant_store.%s: tenant_id raised for entry; rejecting with PERMISSION_DENIED", + operation, + exc_info=True, + ) + return "PERMISSION_DENIED", _permission_denied_message(ref), "correctable" + + if auth_tid is None: + return "PERMISSION_DENIED", _permission_denied_message(ref), "correctable" + if auth_tid != entry_tid: + return "ACCOUNT_NOT_FOUND", _account_not_found_message(ref), "terminal" + return None + async def resolve( self, ref: AccountReference | dict[str, Any] | None, @@ -325,9 +376,9 @@ async def upsert( 1. Compute the entry's tenant via ``resolve_by_ref``. 2. Compare against the auth principal's tenant (``resolve_from_auth(ctx)``, computed once per call). - 3. Unknown ref → ``ACCOUNT_NOT_FOUND``. - 4. Auth tenant ``None`` OR auth tenant != entry tenant → - ``PERMISSION_DENIED`` (fail-closed). + 3. Unknown ref OR an account outside the authenticated tenant → + ``ACCOUNT_NOT_FOUND`` (existence-hiding). + 4. Auth tenant ``None`` → ``PERMISSION_DENIED`` (fail-closed). 5. Otherwise, dispatch to ``upsert_row`` (or no-op ``action='unchanged'`` if no hook). @@ -342,51 +393,14 @@ async def upsert( rows: _BuiltinList[SyncAccountsResultRow] = [] for ref in refs: - try: - entry_account = await _await_maybe(self._resolve_by_ref(ref, resolve_ctx)) - except Exception: - # Per-entry isolation: one bad row must not poison the - # batch. Log server-side; emit PERMISSION_DENIED on the - # wire (don't leak adopter exception detail — could - # carry stack/DB info). - logger.warning( - "tenant_store.upsert: resolve_by_ref raised for entry; " - "rejecting with PERMISSION_DENIED", - exc_info=True, - ) - rows.append( - _build_failed_sync_accounts_row( - ref, "PERMISSION_DENIED", _permission_denied_message(ref) - ) - ) - continue - if entry_account is None: - rows.append( - _build_failed_sync_accounts_row( - ref, "ACCOUNT_NOT_FOUND", _account_not_found_message(ref) - ) - ) - continue - try: - entry_tid = self._tenant_id(entry_account) - except Exception: - logger.warning( - "tenant_store.upsert: tenant_id raised for entry; " - "rejecting with PERMISSION_DENIED", - exc_info=True, - ) - rows.append( - _build_failed_sync_accounts_row( - ref, "PERMISSION_DENIED", _permission_denied_message(ref) - ) - ) - continue - if auth_tid is None or auth_tid != entry_tid: - rows.append( - _build_failed_sync_accounts_row( - ref, "PERMISSION_DENIED", _permission_denied_message(ref) - ) - ) + failure = await self._classify_entry_access( + ref, + resolve_ctx, + auth_tid, + operation="upsert", + ) + if failure is not None: + rows.append(_build_failed_sync_accounts_row(ref, *failure)) continue if self._upsert_row is None: rows.append(_default_unchanged_row(ref)) @@ -443,55 +457,14 @@ async def sync_governance( rows: _BuiltinList[SyncGovernanceResultRow] = [] for entry in entries: - try: - entry_account = await _await_maybe(self._resolve_by_ref(entry.account, resolve_ctx)) - except Exception: - logger.warning( - "tenant_store.sync_governance: resolve_by_ref raised for entry; " - "rejecting with PERMISSION_DENIED", - exc_info=True, - ) - rows.append( - _build_failed_sync_governance_row( - entry, - "PERMISSION_DENIED", - _permission_denied_message(entry.account), - ) - ) - continue - if entry_account is None: - rows.append( - _build_failed_sync_governance_row( - entry, - "ACCOUNT_NOT_FOUND", - _account_not_found_message(entry.account), - ) - ) - continue - try: - entry_tid = self._tenant_id(entry_account) - except Exception: - logger.warning( - "tenant_store.sync_governance: tenant_id raised for entry; " - "rejecting with PERMISSION_DENIED", - exc_info=True, - ) - rows.append( - _build_failed_sync_governance_row( - entry, - "PERMISSION_DENIED", - _permission_denied_message(entry.account), - ) - ) - continue - if auth_tid is None or auth_tid != entry_tid: - rows.append( - _build_failed_sync_governance_row( - entry, - "PERMISSION_DENIED", - _permission_denied_message(entry.account), - ) - ) + failure = await self._classify_entry_access( + entry.account, + resolve_ctx, + auth_tid, + operation="sync_governance", + ) + if failure is not None: + rows.append(_build_failed_sync_governance_row(entry, *failure)) continue if self._sync_governance_row is None: rows.append( diff --git a/src/adcp/decisioning/upstream.py b/src/adcp/decisioning/upstream.py index 4c184d50e..9a1b1ecf4 100644 --- a/src/adcp/decisioning/upstream.py +++ b/src/adcp/decisioning/upstream.py @@ -25,12 +25,21 @@ from __future__ import annotations +import asyncio +from collections import OrderedDict from collections.abc import Awaitable, Callable, Mapping from dataclasses import dataclass from typing import Any, Literal import httpx +from adcp.decisioning.errors import ( + AuthRequiredError, + MediaBuyNotFoundError, + PermissionDeniedError, + RateLimitedError, + ServiceUnavailableError, +) from adcp.decisioning.types import AdcpError #: Per-call routing context forwarded to :class:`DynamicBearer.get_token`. @@ -106,47 +115,30 @@ def _project_status( not_found_code: str, method: str, path: str, - body_text: str, ) -> AdcpError: - """Project an upstream non-2xx status to a spec-conformant AdcpError.""" - snippet = body_text[:200] if body_text else "" - suffix = f" — {snippet}" if snippet else "" - base = f"upstream {method} {path} failed: {status_code}{suffix}" + """Project an upstream status without exposing its untrusted response body.""" + base = f"upstream {method} {path} failed: {status_code}" if status_code == 401: - return AdcpError( - "AUTH_REQUIRED", - message=base, - recovery="terminal", - ) + return AuthRequiredError(message=base) if status_code == 403: - return AdcpError( - "PERMISSION_DENIED", - message=base, - recovery="terminal", - ) + return PermissionDeniedError(message=base) if status_code == 404: + if not_found_code == _DEFAULT_NOT_FOUND_CODE: + return MediaBuyNotFoundError(message=base) return AdcpError( not_found_code, message=base, - recovery="terminal", + recovery="correctable", ) if status_code == 429: - return AdcpError( - "RATE_LIMITED", - message=base, - recovery="transient", - ) + return RateLimitedError(message=base) if status_code >= 500: - return AdcpError( - "SERVICE_UNAVAILABLE", - message=base, - recovery="transient", - ) + return ServiceUnavailableError(message=base) # Any other 4xx → buyer-fixable. return AdcpError( "INVALID_REQUEST", message=base, - recovery="retry_with_changes", + recovery="correctable", ) @@ -270,16 +262,11 @@ async def _request( if response.status_code == 404 and self._treat_404_as_none: return None if response.status_code >= 300: - try: - body_text = response.text - except Exception: # pragma: no cover — defensive - body_text = "" raise _project_status( response.status_code, not_found_code=not_found_code, method=method, path=path, - body_text=body_text, ) if response.status_code == 204 or not response.content: return {} @@ -342,10 +329,11 @@ async def post( # POST 404 is unusual; treat_404_as_none still applies but # callers don't expect None — surface as MEDIA_BUY_NOT_FOUND. if result is None: - raise AdcpError( - not_found_code, - message=f"upstream POST {path} returned 404", - recovery="terminal", + raise _project_status( + 404, + not_found_code=not_found_code, + method="POST", + path=path, ) return result @@ -370,10 +358,11 @@ async def put( not_found_code=not_found_code, ) if result is None: - raise AdcpError( - not_found_code, - message=f"upstream PUT {path} returned 404", - recovery="terminal", + raise _project_status( + 404, + not_found_code=not_found_code, + method="PUT", + path=path, ) return result @@ -398,6 +387,71 @@ async def delete( ) +class UpstreamClientPool: + """Own and reuse upstream clients without invalidating active borrowers. + + The reusable LRU is bounded, but eviction only retires a client: callers + receive raw :class:`UpstreamHttpClient` instances, so the pool cannot know + when an in-flight borrower has finished. Retired clients are therefore + closed together with cached clients by :meth:`aclose` at application + shutdown. This avoids closing a connection pool underneath an active + request while keeping the hot reuse set bounded. + """ + + def __init__(self, *, max_size: int = 128) -> None: + if max_size < 1: + raise ValueError("max_size must be at least 1") + self._max_size = max_size + self._cache: OrderedDict[tuple[Any, ...], UpstreamHttpClient] = OrderedDict() + self._retired: list[UpstreamHttpClient] = [] + + def get( + self, + *, + base_url: str, + auth: UpstreamAuth, + default_headers: Mapping[str, str] | None = None, + timeout: float = 30.0, + treat_404_as_none: bool = True, + ) -> UpstreamHttpClient: + """Return the LRU-cached client for one transport configuration.""" + header_key = tuple( + sorted((name.lower(), value) for name, value in (default_headers or {}).items()) + ) + key = ( + base_url, + id(auth), + header_key, + float(timeout), + bool(treat_404_as_none), + ) + existing = self._cache.get(key) + if existing is not None: + self._cache.move_to_end(key) + return existing + + client = UpstreamHttpClient( + base_url=base_url, + auth=auth, + default_headers=default_headers, + timeout=timeout, + treat_404_as_none=treat_404_as_none, + ) + self._cache[key] = client + while len(self._cache) > self._max_size: + _, retired = self._cache.popitem(last=False) + self._retired.append(retired) + return client + + async def aclose(self) -> None: + """Close every client owned by the pool and reset it for reuse.""" + clients = [*self._cache.values(), *self._retired] + self._cache.clear() + self._retired.clear() + if clients: + await asyncio.gather(*(client.aclose() for client in clients)) + + def create_upstream_http_client( base_url: str, *, @@ -455,6 +509,7 @@ def create_upstream_http_client( "NoAuth", "StaticBearer", "UpstreamAuth", + "UpstreamClientPool", "UpstreamHttpClient", "create_upstream_http_client", ] diff --git a/src/adcp/decisioning/webhook_emit.py b/src/adcp/decisioning/webhook_emit.py index 2ae5c63eb..17f0cdac1 100644 --- a/src/adcp/decisioning/webhook_emit.py +++ b/src/adcp/decisioning/webhook_emit.py @@ -43,7 +43,7 @@ import asyncio import logging import uuid -from typing import TYPE_CHECKING, Any +from typing import TYPE_CHECKING, Any, TypeAlias from adcp.decisioning.account_projection import ( strip_credentials_from_wire_result, @@ -54,7 +54,7 @@ from adcp.webhook_sender import WebhookSender from adcp.webhook_supervisor import WebhookDeliverySupervisor - DeliveryTarget = WebhookSender | WebhookDeliverySupervisor + DeliveryTarget: TypeAlias = WebhookSender | WebhookDeliverySupervisor logger = logging.getLogger(__name__) diff --git a/src/adcp/server/auth.py b/src/adcp/server/auth.py index e98532442..5e5361d06 100644 --- a/src/adcp/server/auth.py +++ b/src/adcp/server/auth.py @@ -80,7 +80,7 @@ async def validate_token(token: str) -> Principal | None: import json import logging import warnings -from collections.abc import Awaitable, Collection, Mapping, Sequence +from collections.abc import Awaitable, Callable, Collection, Mapping, Sequence from contextvars import ContextVar from dataclasses import dataclass, field from typing import TYPE_CHECKING, Any, Literal, Protocol, TypeVar @@ -225,6 +225,28 @@ def __call__(self, token: str) -> Awaitable[Principal | None]: ... REQUEST_STATE_PRINCIPAL = "adcp_auth_principal" REQUEST_STATE_TENANT = "adcp_auth_tenant" REQUEST_STATE_PRINCIPAL_METADATA = "adcp_auth_principal_metadata" +REQUEST_STATE_ROUTED_TENANT = "adcp_routed_tenant" + +# Private metadata keys used by tenant-aware adopters at the skill-dispatch +# boundary. They deliberately distinguish an authenticated principal whose +# token omitted ``tenant_id`` from an unauthenticated request: the former has +# ``AUTHENTICATED_TENANT_METADATA_KEY`` present with a value of ``None``. +AUTHENTICATED_TENANT_METADATA_KEY = "adcp.authenticated_tenant_id" +ROUTED_TENANT_METADATA_KEY = "adcp.routed_tenant_id" + +# The stateful MCP session manager uses this request-scope bit to distinguish +# the middleware's deliberately anonymous discovery bypass from an anonymous +# non-discovery request. Keeping the decision here preserves per-instance +# discovery overrides and subclassed ``is_discovery_request`` policies. +REQUEST_SCOPE_DISCOVERY = "adcp_auth_discovery_request" + + +def _current_routed_tenant_id() -> str | None: + """Return the tenant selected by subdomain routing, when installed.""" + from adcp.server.tenant_router import current_tenant as routed_tenant # noqa: PLC0415 + + tenant = routed_tenant() + return tenant.id if tenant is not None else None def _set_request_state( @@ -245,6 +267,7 @@ def _set_request_state( setattr(state, REQUEST_STATE_PRINCIPAL, principal_identity) setattr(state, REQUEST_STATE_TENANT, tenant_id) setattr(state, REQUEST_STATE_PRINCIPAL_METADATA, principal_metadata) + setattr(state, REQUEST_STATE_ROUTED_TENANT, _current_routed_tenant_id()) def _read_request_state_auth( @@ -432,19 +455,21 @@ def __init__( async def dispatch(self, request: Request, call_next: Any) -> Any: method, tool = await self._peek_jsonrpc(request) + is_discovery = self.is_discovery_request(method, tool) + request.scope[REQUEST_SCOPE_DISCOVERY] = is_discovery principal_token = None tenant_token = None metadata_token = None try: - if self.is_discovery_request(method, tool): + bearer = self._extract_bearer(request) + if is_discovery and not bearer: principal_token = current_principal.set(None) tenant_token = current_tenant.set(None) metadata_token = current_principal_metadata.set(None) _set_request_state(request, None, None, None) return await call_next(request) - bearer = self._extract_bearer(request) if not bearer: if self._allow_unauthenticated: # Network-trust deployment: no bearer is expected on this @@ -686,10 +711,14 @@ def auth_context_factory(meta: RequestMetadata) -> ToolContext: principal_identity: str | None = None tenant_id: str | None = None principal_metadata: dict[str, Any] | None = None + routed_tenant_id: str | None = None if meta.request_context is not None: triple = _read_request_state_auth(meta.request_context) if triple is not None: principal_identity, tenant_id, principal_metadata = triple + state = getattr(meta.request_context, "state", None) + if state is not None and hasattr(state, REQUEST_STATE_ROUTED_TENANT): + routed_tenant_id = getattr(state, REQUEST_STATE_ROUTED_TENANT, None) if principal_identity is None and tenant_id is None and principal_metadata is None: # Either no Request was threaded (stdio MCP, A2A pre-builder # path) or the middleware didn't write to state — fall back to @@ -698,6 +727,10 @@ def auth_context_factory(meta: RequestMetadata) -> ToolContext: principal_identity = current_principal.get() tenant_id = current_tenant.get() principal_metadata = current_principal_metadata.get() + if routed_tenant_id is None: + # A2A dispatch shares the outer ASGI middleware's ContextVar. MCP + # stateful dispatch instead uses the request-state value above. + routed_tenant_id = _current_routed_tenant_id() principal_metadata = principal_metadata or {} combined_metadata: dict[str, Any] = { **principal_metadata, @@ -705,6 +738,7 @@ def auth_context_factory(meta: RequestMetadata) -> ToolContext: "transport": meta.transport, } if principal_identity is not None: + combined_metadata[AUTHENTICATED_TENANT_METADATA_KEY] = tenant_id # Lazy import to keep module-load order safe — decisioning.context # imports adcp.server.base but not adcp.server.auth, so there is no # circular dependency, but hoisting this to module level would create @@ -717,6 +751,8 @@ def auth_context_factory(meta: RequestMetadata) -> ToolContext: principal=principal_identity, credential=None, # explicit None: no synthesis, no DeprecationWarning ) + if routed_tenant_id is not None: + combined_metadata[ROUTED_TENANT_METADATA_KEY] = routed_tenant_id return ToolContext( request_id=meta.request_id, caller_identity=principal_identity, @@ -725,6 +761,39 @@ def auth_context_factory(meta: RequestMetadata) -> ToolContext: ) +async def enforce_authenticated_tenant( + _skill_name: str, + _params: dict[str, Any], + context: ToolContext, + call_next: Callable[[], Awaitable[Any]], +) -> Any: + """Reject a bearer credential used on another routed tenant's host. + + Wire this as :data:`~adcp.server.SkillMiddleware` alongside + :func:`auth_context_factory` and :class:`SubdomainTenantMiddleware`. + Skill middleware runs inside both transports' structured-error boundary, + so ``PERMISSION_DENIED`` has the normal MCP/A2A protocol projection. + + Presence, rather than truthiness, of the authenticated-tenant metadata + key is load-bearing: an authenticated principal with ``tenant_id=None`` + must not be rebound to whichever host the caller selected. + """ + metadata = context.metadata or {} + if ( + AUTHENTICATED_TENANT_METADATA_KEY in metadata + and ROUTED_TENANT_METADATA_KEY in metadata + and metadata[AUTHENTICATED_TENANT_METADATA_KEY] != metadata[ROUTED_TENANT_METADATA_KEY] + ): + from adcp.decisioning import AdcpError # noqa: PLC0415 + + raise AdcpError( + "PERMISSION_DENIED", + message="Bearer credential is not valid for this tenant.", + recovery="correctable", + ) + return await call_next() + + # ------------------------------------------------------------------ # Helpers sellers sometimes need when building their own validator. # ------------------------------------------------------------------ diff --git a/src/adcp/server/mcp_sessions.py b/src/adcp/server/mcp_sessions.py index de3640eed..1122b6d2a 100644 --- a/src/adcp/server/mcp_sessions.py +++ b/src/adcp/server/mcp_sessions.py @@ -8,6 +8,7 @@ from __future__ import annotations +import hashlib import json import logging import time @@ -21,8 +22,10 @@ from anyio.abc import TaskStatus from mcp.server.auth.middleware.bearer_auth import ( AuthenticatedUser, + AuthorizationContext, authorization_context, ) +from mcp.server.auth.provider import AccessToken from mcp.server.runner import serve_loop from mcp.server.streamable_http import MCP_SESSION_ID_HEADER, StreamableHTTPServerTransport from mcp.server.streamable_http_manager import ( @@ -33,9 +36,26 @@ from starlette.responses import Response from starlette.types import Receive, Scope, Send +from adcp.server.auth import REQUEST_SCOPE_DISCOVERY, _read_request_state_auth + logger = logging.getLogger("adcp.server") +@dataclass(frozen=True) +class _UnboundSession: + """A pre-auth session that has not yet seen an authenticated caller.""" + + +@dataclass(frozen=True) +class _ClaimedSession: + """A session permanently bound to its first authenticated caller.""" + + owner: AuthorizationContext + + +_UNBOUND_SESSION = _UnboundSession() + + @dataclass(frozen=True) class MCPSessionStats: """Snapshot of a Streamable HTTP session manager. @@ -97,6 +117,7 @@ def __init__( self._session_last_seen_at: dict[str, float] = {} self._session_creation_events: deque[float] = deque() self._total_sessions_created = 0 + self._session_bindings: dict[str, _UnboundSession | _ClaimedSession] = {} def session_stats(self) -> MCPSessionStats: """Return a point-in-time session snapshot.""" @@ -138,28 +159,22 @@ async def _handle_stateful_request( """ request = Request(scope, receive) request_mcp_session_id = request.headers.get(MCP_SESSION_ID_HEADER) - user = scope.get("user") - requestor = authorization_context(user) if isinstance(user, AuthenticatedUser) else None + requestor = self._requestor(request, scope) if request_mcp_session_id is not None and request_mcp_session_id in self._server_instances: transport = self._server_instances[request_mcp_session_id] - if requestor != self._session_owners.get(request_mcp_session_id): + if not await self._authorize_existing_session( + request_mcp_session_id, + requestor, + auth_middleware_ran=REQUEST_SCOPE_DISCOVERY in scope, + anonymous_discovery=scope.get(REQUEST_SCOPE_DISCOVERY) is True, + ): logger.warning( - "Rejecting request for session %s: credential does not match the one that " - "created the session", + "Rejecting request for session %s: session is unclaimed or credential does " + "not match its owner", request_mcp_session_id[:64], ) - error_body = JSONRPCError( - jsonrpc="2.0", - id=None, - error=ErrorData(code=INVALID_REQUEST, message="Session not found"), - ) - response = Response( - error_body.model_dump_json(by_alias=True, exclude_unset=True), - status_code=HTTPStatus.NOT_FOUND, - media_type="application/json", - ) - await response(scope, receive, send) + await self._send_session_not_found(scope, receive, send) return logger.debug("Session already exists, handling request directly") self._session_last_seen_at[request_mcp_session_id] = time.monotonic() @@ -169,6 +184,7 @@ async def _handle_stateful_request( if transport.is_terminated: self._server_instances.pop(request_mcp_session_id, None) self._session_owners.pop(request_mcp_session_id, None) + self._session_bindings.pop(request_mcp_session_id, None) self._forget_session(request_mcp_session_id) return @@ -199,6 +215,10 @@ async def _handle_stateful_request( assert http_transport.mcp_session_id is not None if requestor is not None: self._session_owners[http_transport.mcp_session_id] = requestor + binding: _UnboundSession | _ClaimedSession = _ClaimedSession(requestor) + else: + binding = _UNBOUND_SESSION + self._session_bindings[http_transport.mcp_session_id] = binding self._server_instances[http_transport.mcp_session_id] = http_transport self._remember_session(http_transport.mcp_session_id) logger.info("Created new MCP stateful transport") @@ -232,6 +252,7 @@ async def run_server( logger.info("MCP stateful session idle timeout") self._server_instances.pop(http_transport.mcp_session_id, None) self._session_owners.pop(http_transport.mcp_session_id, None) + self._session_bindings.pop(http_transport.mcp_session_id, None) self._forget_session(http_transport.mcp_session_id) await http_transport.terminate() except Exception: @@ -248,6 +269,7 @@ async def run_server( ) del self._server_instances[http_transport.mcp_session_id] self._session_owners.pop(http_transport.mcp_session_id, None) + self._session_bindings.pop(http_transport.mcp_session_id, None) self._forget_session(http_transport.mcp_session_id) task_group = getattr(self, "_task_group", None) @@ -271,6 +293,78 @@ async def run_server( ) await response(scope, receive, send) + @staticmethod + def _requestor(request: Request, scope: Scope) -> AuthorizationContext | None: + """Project ADCP request state into MCP's standard principal shape.""" + user = scope.get("user") + if isinstance(user, AuthenticatedUser): + return authorization_context(user) + + triple = _read_request_state_auth(request) + if triple is None: + return None + principal_identity, tenant_id, _metadata = triple + if principal_identity is None: + return None + + # The session manager needs a stable authorization context, not bearer + # material. Constructing MCP's transport-specific shape here keeps the + # generic Starlette auth middleware independent of MCP internals. + token = hashlib.sha256(f"{principal_identity}\0{tenant_id!r}".encode()).hexdigest() + return authorization_context( + AuthenticatedUser( + AccessToken( + token=token, + client_id=principal_identity, + scopes=[], + subject=tenant_id, + ) + ) + ) + + async def _authorize_existing_session( + self, + session_id: str, + requestor: AuthorizationContext | None, + *, + auth_middleware_ran: bool, + anonymous_discovery: bool, + ) -> bool: + """Atomically authorize or first-claim a stateful MCP session.""" + async with self._session_creation_lock: + binding = self._session_bindings.get(session_id) + if isinstance(binding, _ClaimedSession): + return requestor is not None and requestor == binding.owner + if not isinstance(binding, _UnboundSession): + return False + if requestor is not None: + claimed = _ClaimedSession(requestor) + self._session_bindings[session_id] = claimed + self._session_owners[session_id] = requestor + return True + # Auth-less servers retain their historical anonymous behavior. + # Once BearerTokenAuthMiddleware is installed, however, only its + # explicit discovery bypass may reuse an unbound session. + return not auth_middleware_ran or anonymous_discovery + + async def _send_session_not_found( + self, + scope: Scope, + receive: Receive, + send: Send, + ) -> None: + error_body = JSONRPCError( + jsonrpc="2.0", + id=None, + error=ErrorData(code=INVALID_REQUEST, message="Session not found"), + ) + response = Response( + error_body.model_dump_json(by_alias=True, exclude_unset=True), + status_code=HTTPStatus.NOT_FOUND, + media_type="application/json", + ) + await response(scope, receive, send) + def _remember_session(self, session_id: str) -> None: now = time.monotonic() self._session_created_at[session_id] = now diff --git a/src/adcp/server/responses.py b/src/adcp/server/responses.py index c3fe39f83..d5c8ba564 100644 --- a/src/adcp/server/responses.py +++ b/src/adcp/server/responses.py @@ -27,6 +27,7 @@ async def get_products(): from typing import Any from adcp._version import ADCP_MAJOR_VERSION, get_supported_adcp_versions +from adcp.decisioning.account_projection import strip_credentials_from_wire_result from adcp.server.helpers import valid_actions_for_status from adcp.types.canonical_creative import Format, strip_legacy_creative_identity @@ -170,71 +171,38 @@ def _strip_none_values(value: Any) -> Any: def _strip_write_only_fields(value: Any) -> Any: - """Recursively strip write-only credential fields from a wire dict. - - Mirrors :func:`adcp.decisioning.account_projection._project_governance_agent` - at the response-builder layer. The decisioning dispatcher's strip - runs at ``_invoke_platform_method`` for platform methods; this - layer covers adopters who hand-build response payloads via the - ``adcp.server.responses`` builders without going through the - decisioning dispatcher. - - Strips: - - * ``governance_agents[i].authentication`` — write-only credential. - * ``billing_entity.bank`` — write-only bank coordinates. - - Pydantic models are passed through unchanged — adopters using - typed response models are responsible for the strip via - :func:`adcp.decisioning.project_account_for_response` or - equivalent. Loose dicts (the more common case for hand-built - builder calls) get the recursive walk. + """Delegate response sanitization to the canonical account scrubber. + + Public response builders are an independent wire boundary: adopters can + call them without going through the decisioning dispatcher. Keep one + security rule set by routing every serialized value through + :func:`adcp.decisioning.account_projection.strip_credentials_from_wire_result` + under a credential-bearing method name. + + This covers write-only account fields, public-only ``authorization`` + projection, and code-specific error-detail sanitization. The canonical + helper normalizes nested Pydantic models before recursing and does not + mutate the caller's value. """ - if isinstance(value, dict): - out: dict[str, Any] = {} - for key, sub in value.items(): - if key == "governance_agents" and isinstance(sub, list): - projected: list[Any] = [] - for agent in sub: - if isinstance(agent, dict): - projected.append( - { - k: _strip_write_only_fields(v) - for k, v in agent.items() - if k != "authentication" - } - ) - else: - projected.append(agent) - out[key] = projected - elif key == "billing_entity" and isinstance(sub, dict): - out[key] = {k: _strip_write_only_fields(v) for k, v in sub.items() if k != "bank"} - else: - out[key] = _strip_write_only_fields(sub) - return out - if isinstance(value, list): - return [_strip_write_only_fields(v) for v in value] - return value + return strip_credentials_from_wire_result("sync_accounts", value) def _serialize(items: list[Any]) -> list[Any]: """Serialize a list of dicts or Pydantic models to plain dicts. - Loose-dict items (adopters returning ``{**db_record, ...}`` from - a hand-built response builder) get a recursive write-only-field - strip via :func:`_strip_write_only_fields` so - ``governance_agents[i].authentication`` and ``billing_entity.bank`` + Every item gets canonical public-response sanitization via + :func:`_strip_write_only_fields` so + credentials, private authorization metadata, and private error details can't smuggle through, followed by :func:`_strip_none_values` to remove ``null``-valued keys that the bundled JSON schemas declare as - non-nullable (e.g. ``ImageAsset.format``). Pydantic models are - passed through their own ``model_dump(exclude_none=True)`` — the - typed projections at :mod:`adcp.decisioning.account_projection` are - responsible for the write-only strip on that path. + non-nullable (e.g. ``ImageAsset.format``). Pydantic models are first + converted through ``model_dump(exclude_none=True)`` and then scrubbed. """ out: list[Any] = [] for p in items: if hasattr(p, "model_dump"): - out.append(p.model_dump(mode="json", exclude_none=True)) + dumped = p.model_dump(mode="json", exclude_none=True) + out.append(_strip_write_only_fields(dumped)) elif isinstance(p, dict): out.append(_strip_none_values(_strip_write_only_fields(p))) else: diff --git a/src/adcp/types/projections.py b/src/adcp/types/projections.py index 2b432695c..4a6018f3d 100644 --- a/src/adcp/types/projections.py +++ b/src/adcp/types/projections.py @@ -2,11 +2,10 @@ The AdCP spec marks certain fields as ``writeOnly: true`` — present in requests so adopters can populate them, but MUST NOT be echoed in -responses. The clearest case is ``BusinessEntity.bank``: IBANs, BICs, -routing numbers, and account numbers flow into the seller during account -setup and stay there. Pydantic's default serialization round-trips -everything, so an adopter who reuses an internal ``Account`` model on -the response path can leak bank details without realizing it. +responses. ``BusinessEntity.bank`` and notification authentication credentials +flow into the seller during account setup and stay there. Pydantic's default +serialization round-trips everything, so an adopter who reuses an internal +``Account`` model on the response path can leak secrets without realizing it. The projections here type-narrow the write-only fields to ``None``: construction with a non-None value raises ``ValidationError``, and the @@ -26,13 +25,14 @@ import re from collections.abc import Iterator, Mapping -from typing import Any +from typing import Annotated, Any -from pydantic import Field, field_validator +from pydantic import ConfigDict, Field, field_validator from adcp._version import normalize_to_release_precision -from adcp.types import Account, BusinessEntity +from adcp.types import Account, BusinessEntity, NotificationAuthentication, NotificationConfig from adcp.types.capabilities import GeoPostalAreas, LegacyPostalCodeSystem +from adcp.types.variants import SchemaVariant _NATIVE_TO_LEGACY_POSTAL: dict[tuple[str, str], LegacyPostalCodeSystem] = { ("US", "zip"): LegacyPostalCodeSystem.us_zip, @@ -165,6 +165,32 @@ def _reject_bank(cls, v: Any) -> None: return None +class _NotificationAuthenticationResponse(NotificationAuthentication): + """Response projection of legacy notification authentication.""" + + model_config = ConfigDict(extra="forbid") + + credentials: Any = Field(default=None, exclude=True) + + @field_validator("credentials", mode="before") + @classmethod + def _reject_credentials(cls, v: Any) -> None: + if v is not None: + raise ValueError( + "Notification authentication credentials are write-only and " + "must not be included in an AccountResponse. Drop the field " + "before constructing a response, or use to_account_response() " + "to strip it." + ) + return None + + +class _NotificationConfigResponse(NotificationConfig): + """Account notification config with write-only credentials stripped.""" + + authentication: SchemaVariant[_NotificationAuthenticationResponse | None] = None + + class AccountResponse(Account): """Response projection of :class:`Account` — billing_entity is the bank-stripped variant. @@ -177,12 +203,16 @@ class AccountResponse(Account): """ billing_entity: BusinessEntityResponse | None = None + notification_configs: SchemaVariant[ + Annotated[list[_NotificationConfigResponse], Field(max_length=16)] | None + ] = None def to_account_response(account: Account) -> AccountResponse: """Project an internal ``Account`` to its response shape. - Strips ``billing_entity.bank`` and returns an :class:`AccountResponse`. + Strips ``billing_entity.bank`` and notification authentication credentials, + then returns an :class:`AccountResponse`. The remaining fields (legal_name, tax_id, address, contacts, vat_id, registration_number, ext) round-trip unchanged. ``reporting_bucket``, ``governance_agents``, and other non-write-only fields are preserved. @@ -194,6 +224,9 @@ def to_account_response(account: Account) -> AccountResponse: payload = account.model_dump(mode="python") if isinstance(payload.get("billing_entity"), dict): payload["billing_entity"].pop("bank", None) + for config in payload.get("notification_configs") or []: + if isinstance(config, dict) and isinstance(config.get("authentication"), dict): + config["authentication"].pop("credentials", None) return AccountResponse.model_validate(payload) diff --git a/tests/conformance/decisioning/test_pg_buyer_agent_registry.py b/tests/conformance/decisioning/test_pg_buyer_agent_registry.py index 91f3fe1af..8099794aa 100644 --- a/tests/conformance/decisioning/test_pg_buyer_agent_registry.py +++ b/tests/conformance/decisioning/test_pg_buyer_agent_registry.py @@ -181,6 +181,83 @@ def test_resolve_by_credential_returns_none_for_unknown_key(isolated_pool) -> No assert result is None +def test_upsert_rejects_duplicate_credential_identifier(isolated_pool) -> None: + registry = _registry(isolated_pool) + registry.upsert( + BuyerAgent( + agent_url="https://first-buyer/", + display_name="First Buyer", + status="active", + ), + api_key_id="shared-credential", + ) + + with pytest.raises(psycopg.errors.UniqueViolation): + registry.upsert( + BuyerAgent( + agent_url="https://second-buyer/", + display_name="Second Buyer", + status="active", + ), + api_key_id="shared-credential", + ) + + +def test_legacy_duplicate_credentials_fail_closed(isolated_pool) -> None: + """Lookup remains safe before an existing deployment applies the index.""" + pool, table = isolated_pool + registry = _registry(isolated_pool) + with pool.connection() as conn, conn.cursor() as cur: + cur.execute(f"DROP INDEX {table}_api_key_id_uidx") + for suffix in ("one", "two"): + registry.upsert( + BuyerAgent( + agent_url=f"https://buyer-{suffix}/", + display_name=f"Buyer {suffix}", + status="active", + ), + api_key_id="legacy-duplicate", + ) + + result = asyncio.run( + registry.resolve_by_credential( + ApiKeyCredential(kind="api_key", key_id="legacy-duplicate"), + ) + ) + assert result is None + + +def test_create_schema_rejects_legacy_duplicates_before_replacing_index(isolated_pool) -> None: + """Bootstrap reports remediation and preserves the legacy lookup index.""" + pool, table = isolated_pool + registry = _registry(isolated_pool) + with pool.connection() as conn, conn.cursor() as cur: + cur.execute(f"DROP INDEX {table}_api_key_id_uidx") + cur.execute( + f"CREATE INDEX {table}_api_key_id_idx ON {table} (api_key_id) " + "WHERE api_key_id IS NOT NULL" + ) + for suffix in ("one", "two"): + registry.upsert( + BuyerAgent( + agent_url=f"https://legacy-{suffix}/", + display_name=f"Legacy {suffix}", + status="active", + ), + api_key_id="legacy-duplicate", + ) + + with pytest.raises(RuntimeError, match="Rotate or remove duplicate bearer credentials"): + registry.create_schema() + + with pool.connection() as conn, conn.cursor() as cur: + cur.execute( + "SELECT indexname FROM pg_indexes WHERE tablename = %s AND indexname = %s", + (table, f"{table}_api_key_id_idx"), + ) + assert cur.fetchone() == (f"{table}_api_key_id_idx",) + + # ----- upsert (admin path) ----------------------------------------------- diff --git a/tests/conformance/decisioning/test_pg_proposal_store.py b/tests/conformance/decisioning/test_pg_proposal_store.py index 1e56eaf89..df476428a 100644 --- a/tests/conformance/decisioning/test_pg_proposal_store.py +++ b/tests/conformance/decisioning/test_pg_proposal_store.py @@ -79,7 +79,7 @@ async def test_put_draft_then_get_round_trips(store: PgProposalStore) -> None: recipes=recipes, proposal_payload=payload, ) - record = await store.get("p1") + record = await store.get("p1", expected_account_id="acct_a") assert record is not None assert record.proposal_id == "p1" assert record.account_id == "acct_a" @@ -115,7 +115,7 @@ async def test_commit_promotes_draft_to_committed(store: PgProposalStore) -> Non proposal_payload={"committed": True}, expected_account_id="acct_a", ) - record = await store.get("p1") + record = await store.get("p1", expected_account_id="acct_a") assert record is not None assert record.state == ProposalState.COMMITTED assert record.expires_at == expires @@ -158,7 +158,7 @@ async def test_commit_idempotent_on_equal_values(store: PgProposalStore) -> None await store.commit( "p1", expires_at=expires, proposal_payload=payload, expected_account_id="acct_a" ) - record = await store.get("p1") + record = await store.get("p1", expected_account_id="acct_a") assert record is not None assert record.state == ProposalState.COMMITTED @@ -207,7 +207,7 @@ async def test_reserve_finalize_round_trip(store: PgProposalStore) -> None: reserved = await store.try_reserve_consumption("p1", expected_account_id="acct_a") assert reserved.state == ProposalState.CONSUMING await store.finalize_consumption("p1", media_buy_id="mb_1", expected_account_id="acct_a") - record = await store.get("p1") + record = await store.get("p1", expected_account_id="acct_a") assert record is not None assert record.state == ProposalState.CONSUMED assert record.media_buy_id == "mb_1" @@ -330,7 +330,7 @@ async def test_expires_at_round_trip_preserves_utc(store: PgProposalStore) -> No ) expires = datetime.now(timezone.utc) + timedelta(days=7) await store.commit("p1", expires_at=expires, proposal_payload={}, expected_account_id="acct_a") - record = await store.get("p1") + record = await store.get("p1", expected_account_id="acct_a") assert record is not None assert record.expires_at is not None assert record.expires_at.tzinfo is not None diff --git a/tests/test_account_projections.py b/tests/test_account_projections.py index dfee09307..4deb8c256 100644 --- a/tests/test_account_projections.py +++ b/tests/test_account_projections.py @@ -168,3 +168,63 @@ def test_to_account_response_preserves_reporting_bucket() -> None: assert response.reporting_bucket is not None assert response.reporting_bucket.bucket == "reports-acme-prod" assert response.reporting_bucket.file_retention_days == 30 + + +# ---- notification_configs — write-only authentication credentials guard ---- + + +def test_account_response_rejects_notification_credentials() -> None: + """Response-shaped accounts cannot be constructed with webhook secrets.""" + with pytest.raises(ValidationError) as excinfo: + AccountResponse.model_validate( + { + "account_id": "acct-1", + "name": "Acme", + "status": "active", + "notification_configs": [ + { + "subscriber_id": "buyer-primary", + "url": "https://buyer.example/webhooks", + "event_types": ["creative.status_changed"], + "authentication": { + "schemes": ["Bearer"], + "credentials": "secret-token-that-is-at-least-32-chars", + }, + } + ], + } + ) + + assert any( + "credentials" in str(loc) for error in excinfo.value.errors() for loc in error["loc"] + ) + + +def test_to_account_response_strips_notification_credentials() -> None: + """Projection keeps subscription state and scheme but drops its secret.""" + internal = Account.model_validate( + { + "account_id": "acct-1", + "name": "Acme", + "status": "active", + "notification_configs": [ + { + "subscriber_id": "buyer-primary", + "url": "https://buyer.example/webhooks", + "event_types": ["creative.status_changed"], + "authentication": { + "schemes": ["Bearer"], + "credentials": "secret-token-that-is-at-least-32-chars", + }, + "active": True, + } + ], + } + ) + + dumped = to_account_response(internal).model_dump(mode="json", exclude_none=True) + config = dumped["notification_configs"][0] + assert config["subscriber_id"] == "buyer-primary" + assert config["active"] is True + assert config["authentication"]["schemes"] == ["Bearer"] + assert "credentials" not in config["authentication"] diff --git a/tests/test_account_v3_wire.py b/tests/test_account_v3_wire.py index 22b544ebc..9de1b528f 100644 --- a/tests/test_account_v3_wire.py +++ b/tests/test_account_v3_wire.py @@ -317,6 +317,31 @@ def test_account_authorization_projection_strips_accidental_secrets() -> None: assert "conn_private" not in str(scrubbed) +def test_loose_account_projection_strips_notification_credentials() -> None: + secret = "notification-secret-that-must-never-echo" + payload = { + "accounts": [ + { + "account_id": "acct_1", + "notification_configs": [ + { + "subscriber_id": "buyer-primary", + "authentication": { + "schemes": ["HMAC-SHA256"], + "credentials": secret, + }, + } + ], + } + ] + } + + scrubbed = strip_credentials_from_wire_result("list_accounts", payload) + authentication = scrubbed["accounts"][0]["notification_configs"][0]["authentication"] + assert authentication == {"schemes": ["HMAC-SHA256"]} + assert secret not in str(scrubbed) + + @pytest.mark.asyncio async def test_handler_list_accounts_returns_ads_and_publisher_identity_grants() -> None: class _TikTokAccountStore: diff --git a/tests/test_buyer_agent_registry_cache.py b/tests/test_buyer_agent_registry_cache.py index 793880e91..bff8faa6a 100644 --- a/tests/test_buyer_agent_registry_cache.py +++ b/tests/test_buyer_agent_registry_cache.py @@ -314,9 +314,8 @@ async def test_rate_limit_refills_over_time() -> None: @pytest.mark.asyncio -async def test_rate_limit_isolates_distinct_lookup_keys() -> None: - """Each ``(tenant, lookup_key)`` gets its own bucket — exhausting - one does not affect another.""" +async def test_rate_limit_aggregate_blocks_rotating_lookup_keys() -> None: + """Fresh identifiers cannot bypass the tenant's aggregate budget.""" inner = FakeRegistry() clock = FakeClock(start=0.0) limiter = RateLimitedBuyerAgentRegistry( @@ -327,10 +326,120 @@ async def test_rate_limit_isolates_distinct_lookup_keys() -> None: await limiter.resolve_by_agent_url("https://agent-A/") with pytest.raises(AdcpError): - await limiter.resolve_by_agent_url("https://agent-A/") + await limiter.resolve_by_agent_url("https://agent-B/") - # Different key — fresh bucket. + +@pytest.mark.asyncio +async def test_rate_limit_bucket_state_is_bounded() -> None: + limiter = RateLimitedBuyerAgentRegistry( + FakeRegistry(), + rps_per_tenant=100.0, + burst=100.0, + rps_per_lookup=100.0, + max_buckets=4, + time_source=FakeClock(start=0.0), + ) + for suffix in ("A", "B", "C", "D"): + await limiter.resolve_by_agent_url(f"https://agent-{suffix}/") + tenant_buckets = limiter._buckets[None] + assert 1 + len(tenant_buckets.lookups) == 4 + assert set(tenant_buckets.lookups) == { + "agent_url:https://agent-B/", + "agent_url:https://agent-C/", + "agent_url:https://agent-D/", + } + + +@pytest.mark.asyncio +async def test_rate_limit_bucket_cap_is_isolated_per_tenant( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """One tenant's lookup identities cannot crowd out another tenant.""" + tenant = "tenant-a" + monkeypatch.setattr( + "adcp.decisioning.registry_cache._current_tenant_id", + lambda: tenant, + ) + limiter = RateLimitedBuyerAgentRegistry( + FakeRegistry(), + rps_per_tenant=100.0, + rps_per_lookup=100.0, + max_buckets=3, + time_source=FakeClock(start=0.0), + ) + + await limiter.resolve_by_agent_url("https://agent-A/") await limiter.resolve_by_agent_url("https://agent-B/") + await limiter.resolve_by_agent_url("https://agent-C/") + + tenant = "tenant-b" + await limiter.resolve_by_agent_url("https://agent-C/") + assert set(limiter._buckets) == {"tenant-a", "tenant-b"} + + +@pytest.mark.asyncio +async def test_rate_limit_hot_key_does_not_drain_tenant_aggregate() -> None: + """Rejected hot-key probes leave capacity for unrelated lookups.""" + limiter = RateLimitedBuyerAgentRegistry( + FakeRegistry(), + rps_per_tenant=2.0, + burst=2.0, + rps_per_lookup=1.0, + lookup_burst=1.0, + time_source=FakeClock(start=0.0), + ) + + await limiter.resolve_by_agent_url("https://hot-key/") + for _ in range(5): + with pytest.raises(AdcpError): + await limiter.resolve_by_agent_url("https://hot-key/") + + await limiter.resolve_by_agent_url("https://unrelated-key/") + + +@pytest.mark.asyncio +async def test_rate_limit_idle_buckets_expire() -> None: + clock = FakeClock(start=0.0) + limiter = RateLimitedBuyerAgentRegistry( + FakeRegistry(), + rps_per_tenant=100.0, + rps_per_lookup=100.0, + max_buckets=4, + bucket_idle_ttl_seconds=10.0, + time_source=clock, + ) + await limiter.resolve_by_agent_url("https://agent-A/") + await limiter.resolve_by_agent_url("https://agent-B/") + assert 1 + len(limiter._buckets[None].lookups) == 3 + clock.advance(11.0) + await limiter.resolve_by_agent_url("https://agent-C/") + assert 1 + len(limiter._buckets[None].lookups) == 2 + + +@pytest.mark.asyncio +async def test_rate_limit_active_tenant_reclaims_only_idle_lookup_slots() -> None: + clock = FakeClock(start=0.0) + limiter = RateLimitedBuyerAgentRegistry( + FakeRegistry(), + rps_per_tenant=100.0, + rps_per_lookup=100.0, + max_buckets=3, + bucket_idle_ttl_seconds=10.0, + time_source=clock, + ) + await limiter.resolve_by_agent_url("https://stale/") + await limiter.resolve_by_agent_url("https://active/") + clock.advance(6.0) + await limiter.resolve_by_agent_url("https://active/") + clock.advance(5.0) + + await limiter.resolve_by_agent_url("https://replacement/") + lookup_keys = set(limiter._buckets[None].lookups) + assert "agent_url:https://stale/" not in lookup_keys + assert lookup_keys == { + "agent_url:https://active/", + "agent_url:https://replacement/", + } # ----- Audit emission: every outcome fires an event -------------------- @@ -612,6 +721,21 @@ def test_rate_limit_rejects_zero_rps() -> None: RateLimitedBuyerAgentRegistry(FakeRegistry(), rps_per_tenant=0.0) +def test_rate_limit_rejects_idle_ttl_shorter_than_full_refill() -> None: + with pytest.raises(ValueError, match="burst / rps_per_tenant"): + RateLimitedBuyerAgentRegistry( + FakeRegistry(), + rps_per_tenant=1.0, + burst=11.0, + bucket_idle_ttl_seconds=10.0, + ) + + +def test_rate_limit_rejects_lookup_burst_without_lookup_rate() -> None: + with pytest.raises(ValueError, match="lookup_burst requires rps_per_lookup"): + RateLimitedBuyerAgentRegistry(FakeRegistry(), lookup_burst=2.0) + + # ----- clear_sync: mutation-observer entry point --------------------- diff --git a/tests/test_credential_leak_strip.py b/tests/test_credential_leak_strip.py index a908b853a..0f570ecfc 100644 --- a/tests/test_credential_leak_strip.py +++ b/tests/test_credential_leak_strip.py @@ -48,7 +48,11 @@ from adcp.decisioning.types import TaskHandoff from adcp.decisioning.webhook_emit import maybe_emit_sync_completion from adcp.server.base import ToolContext -from adcp.server.responses import sync_governance_response +from adcp.server.responses import ( + media_buys_response, + sync_accounts_response, + sync_governance_response, +) # --------------------------------------------------------------------------- # Shared helpers @@ -66,6 +70,11 @@ def executor() -> Any: _IBAN = "DE89370400440532013000" +class _EntityWithBank(BaseModel): + legal_name: str + bank: dict[str, str] + + def _governance_response_with_credentials() -> dict[str, Any]: """Wire-shape ``sync_governance`` response carrying credentials. @@ -263,6 +272,31 @@ def test_strip_credentials_walks_recursively() -> None: assert "authentication" not in str(out) +def test_dispatch_scrubber_strips_bank_from_nested_pydantic_entities() -> None: + entity = _EntityWithBank( + legal_name="Acme Inc.", + bank={"iban": _IBAN}, + ) + out = strip_credentials_from_wire_result( + "sync_accounts", + {"accounts": [{"billing_entity": entity, "invoice_recipient": entity}]}, + ) + account = out["accounts"][0] + assert account["billing_entity"] == {"legal_name": "Acme Inc."} + assert account["invoice_recipient"] == {"legal_name": "Acme Inc."} + assert _IBAN not in str(out) + + +def test_media_buys_builder_strips_typed_invoice_recipient_bank() -> None: + entity = _EntityWithBank( + legal_name="Acme Inc.", + bank={"iban": _IBAN}, + ) + response = media_buys_response([{"media_buy_id": "mb-1", "invoice_recipient": entity}]) + assert response["media_buys"][0]["invoice_recipient"] == {"legal_name": "Acme Inc."} + assert _IBAN not in str(response) + + # --------------------------------------------------------------------------- # H2 — sync-completion webhook # --------------------------------------------------------------------------- @@ -611,6 +645,17 @@ def test_sync_accounts_response_builder_round_trip_strip() -> None: }, } ], + "notification_configs": [ + { + "subscriber_id": "buyer-primary", + "url": "https://buyer.example/webhooks", + "event_types": ["creative.status_changed"], + "authentication": { + "schemes": ["Bearer"], + "credentials": _BEARER, + }, + } + ], } ], ) @@ -619,9 +664,127 @@ def test_sync_accounts_response_builder_round_trip_strip() -> None: assert _BEARER not in str(response) assert "bank" not in serialized["billing_entity"] assert "authentication" not in serialized["governance_agents"][0] + notification_auth = serialized["notification_configs"][0]["authentication"] + assert notification_auth == {"schemes": ["Bearer"]} assert serialized["billing_entity"]["legal_name"] == "Acme Inc." +def test_response_builder_scrubs_notification_credentials_from_pydantic_models() -> None: + """Request-capable Pydantic models are not trusted on a response edge.""" + from adcp.server.responses import sync_accounts_response + from adcp.types import Account + + account = Account.model_validate( + { + "account_id": "acct_1", + "name": "Acme", + "status": "active", + "notification_configs": [ + { + "subscriber_id": "buyer-primary", + "url": "https://buyer.example/webhooks", + "event_types": ["creative.status_changed"], + "authentication": { + "schemes": ["Bearer"], + "credentials": _BEARER, + }, + } + ], + } + ) + + response = sync_accounts_response([account]) # type: ignore[list-item] + authentication = response["accounts"][0]["notification_configs"][0]["authentication"] + assert authentication == {"schemes": ["Bearer"]} + assert _BEARER not in str(response) + + +@pytest.mark.parametrize( + "builder", + [sync_accounts_response, sync_governance_response], +) +def test_account_response_builders_match_canonical_public_sanitizer(builder: Any) -> None: + """Public builders must use the canonical account/error allowlists.""" + item = { + "account_id": "acct_1", + "authorization": { + "allowed_tasks": ["list_accounts"], + "oauth": {"access_token": _BEARER}, + "internal_connection_id": "conn_private", + }, + "errors": [ + { + "code": "AUTHORIZATION_REQUIRED", + "message": "Authorize the downstream connection", + "details": { + "missing_connections": [ + { + "provider": "social", + "connection_type": "publisher_identity", + "authorization_url": "https://seller.example/connect", + "resource_ref": { + "identity_id": "creator_1", + "internal_user_id": "private", + }, + "client_secret": _BEARER, + } + ], + "debug": {"token": _BEARER}, + }, + } + ], + } + expected = strip_credentials_from_wire_result("sync_accounts", item) + + response = builder([item]) + assert response["accounts"][0] == expected + assert response["accounts"][0]["authorization"] == {"allowed_tasks": ["list_accounts"]} + connection = response["accounts"][0]["errors"][0]["details"]["missing_connections"][0] + assert connection["resource_ref"] == {"identity_id": "creator_1"} + assert _BEARER not in str(response) + assert "internal_connection_id" not in str(response) + assert "client_secret" not in str(response) + assert "debug" not in str(response) + + +def test_account_response_builder_sanitizes_pydantic_errors_in_loose_dict() -> None: + """Typed nested errors cannot bypass the code-specific detail allowlist.""" + + class _TypedError(BaseModel): + code: str + message: str + details: dict[str, Any] + + typed_error = _TypedError( + code="AUTHORIZATION_REQUIRED", + message="Authorize the downstream connection", + details={ + "missing_connections": [ + { + "provider": "social", + "connection_type": "publisher_identity", + "authorization_url": "https://seller.example/connect", + "resource_ref": { + "identity_id": "creator_1", + "internal_user_id": "private", + }, + "client_secret": _BEARER, + } + ], + "debug": {"token": _BEARER}, + }, + ) + + response = sync_accounts_response([{"account_id": "acct_1", "errors": [typed_error]}]) + error = response["accounts"][0]["errors"][0] + assert error["details"]["missing_connections"][0]["resource_ref"] == { + "identity_id": "creator_1" + } + assert _BEARER not in str(response) + assert "client_secret" not in str(response) + assert "debug" not in str(response) + + # --------------------------------------------------------------------------- # M3 — ctx_metadata fail-closed gate # --------------------------------------------------------------------------- diff --git a/tests/test_decisioning_capabilities_submodule.py b/tests/test_decisioning_capabilities_submodule.py index e47e14026..dc3b71898 100644 --- a/tests/test_decisioning_capabilities_submodule.py +++ b/tests/test_decisioning_capabilities_submodule.py @@ -335,6 +335,37 @@ class _TestPlatform(DecisioningPlatform): ) +def test_dual_transport_serve_registers_upstream_pool_shutdown() -> None: + """The framework-owned lifespan drains platform upstream clients.""" + import sys + import unittest.mock as mock + + from adcp.decisioning import ( + DecisioningCapabilities, + DecisioningPlatform, + SingletonAccounts, + ) + from adcp.decisioning.capabilities import SupportedProtocol + from adcp.decisioning.serve import serve + + class _TestPlatform(DecisioningPlatform): + capabilities = DecisioningCapabilities( + supported_protocols=[SupportedProtocol.media_buy], + supported_billing=["operator"], + ) + accounts = SingletonAccounts(account_id="test") + + platform = _TestPlatform() + server_serve_mod = sys.modules["adcp.server.serve"] + with mock.patch.object(server_serve_mod, "serve") as serve_mock: + serve(platform, transport="both") + + shutdown_hooks = serve_mock.call_args.kwargs["on_shutdown"] + assert len(shutdown_hooks) == 1 + assert shutdown_hooks[0].__self__ is platform + assert shutdown_hooks[0].__func__ is platform.aclose_upstream_clients.__func__ + + def test_signals_features_and_content_standards_re_exported() -> None: """``SignalsFeatures`` (codegen ``Features2`` for ``Signals.features``) and ``ContentStandards`` (the ``MediaBuy.content_standards`` type, which diff --git a/tests/test_mcp_stateful_session.py b/tests/test_mcp_stateful_session.py index 90415760c..a17f19abe 100644 --- a/tests/test_mcp_stateful_session.py +++ b/tests/test_mcp_stateful_session.py @@ -401,6 +401,244 @@ async def get_products( assert received["tenant_id"] == "t-acme" +@pytest.mark.asyncio +async def test_stateful_session_is_bound_to_authenticated_principal() -> None: + """A second valid bearer principal cannot attach to another session.""" + from adcp.server import ( + BearerTokenAuthMiddleware, + Principal, + create_mcp_server, + validator_from_token_map, + ) + + mcp = create_mcp_server( + _BareHandler(), + name="t", + advertise_all=True, + allowed_hosts=["localhost", "127.0.0.1"], + ) + app = mcp.streamable_http_app() + app.add_middleware( + BearerTokenAuthMiddleware, + validate_token=validator_from_token_map( + { + "token-alice": Principal(caller_identity="alice", tenant_id="tenant-a"), + "token-bob": Principal(caller_identity="bob", tenant_id="tenant-b"), + } + ), + ) + base_headers = { + "content-type": "application/json", + "accept": "application/json, text/event-stream", + } + + async with LifespanManager(app): + async with httpx.AsyncClient( + transport=httpx.ASGITransport(app=app), + base_url="http://localhost", + follow_redirects=True, + ) as client: + init = await client.post( + "/mcp/", + json={ + "jsonrpc": "2.0", + "id": 0, + "method": "initialize", + "params": { + "protocolVersion": "2025-06-18", + "capabilities": {}, + "clientInfo": {"name": "alice", "version": "1"}, + }, + }, + headers={**base_headers, "authorization": "Bearer token-alice"}, + ) + assert init.status_code == 200, init.text + session_id = init.headers["mcp-session-id"] + + hijack = await client.post( + "/mcp/", + json={"jsonrpc": "2.0", "id": 1, "method": "tools/list", "params": {}}, + headers={ + **base_headers, + "authorization": "Bearer token-bob", + "mcp-session-id": session_id, + }, + ) + assert hijack.status_code == 404 + assert "Session not found" in hijack.text + + owner = await client.post( + "/mcp/", + json={"jsonrpc": "2.0", "id": 2, "method": "tools/list", "params": {}}, + headers={ + **base_headers, + "authorization": "Bearer token-alice", + "mcp-session-id": session_id, + }, + ) + assert owner.status_code == 200, owner.text + + anonymous = await client.post( + "/mcp/", + json={"jsonrpc": "2.0", "id": 3, "method": "tools/list", "params": {}}, + headers={**base_headers, "mcp-session-id": session_id}, + ) + assert anonymous.status_code == 404 + assert "Session not found" in anonymous.text + + +@pytest.mark.asyncio +async def test_pre_auth_session_allows_discovery_then_first_authenticated_caller_claims() -> None: + """Pre-auth initialize is unbound until the first valid principal arrives.""" + from adcp.server import ( + BearerTokenAuthMiddleware, + Principal, + create_mcp_server, + validator_from_token_map, + ) + + mcp = create_mcp_server( + _BareHandler(), + name="t", + advertise_all=True, + allowed_hosts=["localhost", "127.0.0.1"], + ) + app = mcp.streamable_http_app() + app.add_middleware( + BearerTokenAuthMiddleware, + validate_token=validator_from_token_map( + { + "token-alice": Principal(caller_identity="alice", tenant_id="tenant-a"), + "token-bob": Principal(caller_identity="bob", tenant_id="tenant-b"), + } + ), + ) + base_headers = { + "content-type": "application/json", + "accept": "application/json, text/event-stream", + } + + async with LifespanManager(app): + async with httpx.AsyncClient( + transport=httpx.ASGITransport(app=app), + base_url="http://localhost", + follow_redirects=True, + ) as client: + init = await client.post( + "/mcp/", + json={ + "jsonrpc": "2.0", + "id": 0, + "method": "initialize", + "params": { + "protocolVersion": "2025-06-18", + "capabilities": {}, + "clientInfo": {"name": "pre-auth", "version": "1"}, + }, + }, + headers=base_headers, + ) + assert init.status_code == 200, init.text + session_id = init.headers["mcp-session-id"] + + discovery = await client.post( + "/mcp/", + json={"jsonrpc": "2.0", "id": 1, "method": "tools/list", "params": {}}, + headers={**base_headers, "mcp-session-id": session_id}, + ) + assert discovery.status_code == 200, discovery.text + + claim = await client.post( + "/mcp/", + json={"jsonrpc": "2.0", "id": 2, "method": "tools/list", "params": {}}, + headers={ + **base_headers, + "authorization": "Bearer token-alice", + "mcp-session-id": session_id, + }, + ) + assert claim.status_code == 200, claim.text + + for request_headers in ( + {**base_headers, "mcp-session-id": session_id}, + { + **base_headers, + "authorization": "Bearer token-bob", + "mcp-session-id": session_id, + }, + ): + rejected = await client.post( + "/mcp/", + json={ + "jsonrpc": "2.0", + "id": 3, + "method": "tools/list", + "params": {}, + }, + headers=request_headers, + ) + assert rejected.status_code == 404 + assert "Session not found" in rejected.text + + +@pytest.mark.asyncio +async def test_pre_auth_session_rejects_anonymous_non_discovery_reuse() -> None: + """Network-trust bypass cannot turn an unbound session into anonymous access.""" + from adcp.server import BearerTokenAuthMiddleware, create_mcp_server + + mcp = create_mcp_server( + _BareHandler(), + name="t", + advertise_all=True, + allowed_hosts=["localhost", "127.0.0.1"], + ) + app = mcp.streamable_http_app() + app.add_middleware( + BearerTokenAuthMiddleware, + validate_token=lambda _token: None, + allow_unauthenticated=True, + ) + base_headers = { + "content-type": "application/json", + "accept": "application/json, text/event-stream", + } + + async with LifespanManager(app): + async with httpx.AsyncClient( + transport=httpx.ASGITransport(app=app), + base_url="http://localhost", + follow_redirects=True, + ) as client: + init = await client.post( + "/mcp/", + json={ + "jsonrpc": "2.0", + "id": 0, + "method": "initialize", + "params": { + "protocolVersion": "2025-06-18", + "capabilities": {}, + "clientInfo": {"name": "pre-auth", "version": "1"}, + }, + }, + headers=base_headers, + ) + session_id = init.headers["mcp-session-id"] + + rejected = await client.post( + "/mcp/", + json={ + "jsonrpc": "2.0", + "id": 1, + "method": "tools/call", + "params": {"name": "get_products", "arguments": {}}, + }, + headers={**base_headers, "mcp-session-id": session_id}, + ) + assert rejected.status_code == 404 + assert "Session not found" in rejected.text + + @pytest.mark.asyncio async def test_stateful_rejects_request_without_session_id() -> None: """Inverse of the above — without ``Mcp-Session-Id`` the upstream diff --git a/tests/test_pg_buyer_agent_registry_unit.py b/tests/test_pg_buyer_agent_registry_unit.py new file mode 100644 index 000000000..1ec13bb4e --- /dev/null +++ b/tests/test_pg_buyer_agent_registry_unit.py @@ -0,0 +1,90 @@ +"""Database-independent security tests for PgBuyerAgentRegistry SQL paths.""" + +from __future__ import annotations + +from typing import Any + +import pytest + +from adcp.decisioning.pg import buyer_agent_registry as registry_module +from adcp.decisioning.pg.buyer_agent_registry import PgBuyerAgentRegistry + + +class _Cursor: + def __init__(self, rows: list[tuple[Any, ...]] | None = None) -> None: + self.rows = rows or [] + self.queries: list[str] = [] + + def __enter__(self) -> _Cursor: + return self + + def __exit__(self, *args: object) -> None: + return None + + def execute(self, query: str, params: object = None) -> None: + del params + self.queries.append(query) + + def fetchall(self) -> list[tuple[Any, ...]]: + return self.rows + + +class _Connection: + def __init__(self, cursor: _Cursor) -> None: + self._cursor = cursor + + def __enter__(self) -> _Connection: + return self + + def __exit__(self, *args: object) -> None: + return None + + def cursor(self) -> _Cursor: + return self._cursor + + +class _Pool: + def __init__(self, cursor: _Cursor) -> None: + self._connection = _Connection(cursor) + + def connection(self) -> _Connection: + return self._connection + + +@pytest.fixture(autouse=True) +def _enable_optional_pg_module(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(registry_module, "PG_AVAILABLE", True) + + +def test_ambiguous_legacy_credential_mapping_fails_closed() -> None: + cursor = _Cursor(rows=[("first",), ("second",)]) + registry = PgBuyerAgentRegistry(pool=_Pool(cursor)) # type: ignore[arg-type] + + assert registry._sync_lookup_by_api_key_id("shared") is None + assert "LIMIT 2" in cursor.queries[0] + + +def test_schema_bootstrap_creates_partial_unique_credential_index() -> None: + cursor = _Cursor() + registry = PgBuyerAgentRegistry(pool=_Pool(cursor)) # type: ignore[arg-type] + + registry.create_schema() + + ddl = "\n".join(cursor.queries) + assert "CREATE UNIQUE INDEX IF NOT EXISTS" in ddl + assert "api_key_id_uidx" in ddl + assert "WHERE api_key_id IS NOT NULL" in ddl + assert "HAVING COUNT(*) > 1" in ddl + assert "DROP INDEX IF EXISTS adcp_buyer_agents_api_key_id_idx" in ddl + + +def test_schema_bootstrap_rejects_legacy_duplicate_credentials() -> None: + cursor = _Cursor(rows=[("shared-credential", 2)]) + registry = PgBuyerAgentRegistry(pool=_Pool(cursor)) # type: ignore[arg-type] + + with pytest.raises(RuntimeError, match="Rotate or remove duplicate bearer credentials"): + registry.create_schema() + + ddl = "\n".join(cursor.queries) + assert "CREATE UNIQUE INDEX" not in ddl + assert "DROP INDEX" not in ddl diff --git a/tests/test_proposal_auto_commit.py b/tests/test_proposal_auto_commit.py index 374aef594..3270d3bc2 100644 --- a/tests/test_proposal_auto_commit.py +++ b/tests/test_proposal_auto_commit.py @@ -102,7 +102,7 @@ async def test_auto_commit_promotes_draft_to_committed() -> None: } await maybe_persist_draft_after_get_products(platform, response, _ctx()) - record = await store.get("p1") + record = await store.get("p1", expected_account_id="acct-1") assert record is not None assert record.state is ProposalState.COMMITTED @@ -120,7 +120,7 @@ async def test_auto_commit_off_leaves_record_in_draft() -> None: response = {"products": [], "proposals": [{"proposal_id": "p1"}]} await maybe_persist_draft_after_get_products(platform, response, _ctx()) - record = await store.get("p1") + record = await store.get("p1", expected_account_id="acct-1") assert record is not None assert record.state is ProposalState.DRAFT @@ -146,7 +146,7 @@ async def test_auto_commit_expires_at_uses_capability_ttl() -> None: ) after = datetime.now(timezone.utc).timestamp() - record = await store.get("p1") + record = await store.get("p1", expected_account_id="acct-1") assert record is not None assert record.expires_at is not None # expires_at should be ~now + ttl, within the test's wall-clock window. @@ -174,7 +174,7 @@ async def test_auto_commit_handles_multiple_proposals_in_one_response() -> None: ) for pid in ("p1", "p2", "p3"): - record = await store.get(pid) + record = await store.get(pid, expected_account_id="acct-1") assert record is not None, f"missing proposal {pid}" assert ( record.state is ProposalState.COMMITTED @@ -283,6 +283,6 @@ async def test_catalog_mode_store_wired_manager_unwired_no_auto_commit() -> None _ctx(), ) - record = await store.get("p1") + record = await store.get("p1", expected_account_id="acct-1") assert record is not None assert record.state is ProposalState.DRAFT diff --git a/tests/test_proposal_store.py b/tests/test_proposal_store.py index d3e5c0019..661d48a1e 100644 --- a/tests/test_proposal_store.py +++ b/tests/test_proposal_store.py @@ -116,7 +116,7 @@ async def test_put_draft_then_get_round_trips(store: InMemoryProposalStore) -> N recipes=recipes, proposal_payload=payload, ) - record = await store.get("p1") + record = await store.get("p1", expected_account_id="acct_a") assert record is not None assert record.proposal_id == "p1" assert record.account_id == "acct_a" @@ -130,7 +130,7 @@ async def test_put_draft_then_get_round_trips(store: InMemoryProposalStore) -> N @pytest.mark.asyncio async def test_get_unknown_proposal_returns_none(store: InMemoryProposalStore) -> None: - assert await store.get("does-not-exist") is None + assert await store.get("does-not-exist", expected_account_id="acct_a") is None @pytest.mark.asyncio @@ -148,7 +148,7 @@ async def test_put_draft_overwrites_existing_draft(store: InMemoryProposalStore) recipes={"prod_1": _DemoRecipe(line_item_id="li_2")}, proposal_payload={"v": 2}, ) - record = await store.get("p1") + record = await store.get("p1", expected_account_id="acct_a") assert record is not None assert record.state == ProposalState.DRAFT assert record.recipes["prod_1"].line_item_id == "li_2" # type: ignore[attr-defined] @@ -199,7 +199,7 @@ async def test_commit_promotes_draft_to_committed(store: InMemoryProposalStore) await store.commit( "p1", expires_at=expires, proposal_payload={"committed": True}, expected_account_id="acct_a" ) - record = await store.get("p1") + record = await store.get("p1", expected_account_id="acct_a") assert record is not None assert record.state == ProposalState.COMMITTED assert record.expires_at == expires @@ -217,7 +217,7 @@ async def test_commit_idempotent_on_equal_payload(store: InMemoryProposalStore) await store.commit( "p1", expires_at=expires, proposal_payload={"x": 1}, expected_account_id="acct_a" ) - record = await store.get("p1") + record = await store.get("p1", expected_account_id="acct_a") assert record is not None assert record.state == ProposalState.COMMITTED @@ -286,7 +286,7 @@ async def test_mark_consumed_records_media_buy_back_reference( expected_account_id="acct_a", ) await store.mark_consumed("p1", media_buy_id="mb_42", expected_account_id="acct_a") - record = await store.get("p1") + record = await store.get("p1", expected_account_id="acct_a") assert record is not None assert record.state == ProposalState.CONSUMED assert record.media_buy_id == "mb_42" @@ -363,6 +363,31 @@ async def test_get_cross_tenant_returns_none(store: InMemoryProposalStore) -> No assert await store.get("p1", expected_account_id="acct_a") is not None +@pytest.mark.asyncio +async def test_same_proposal_id_is_isolated_by_account(store: InMemoryProposalStore) -> None: + """Two accounts may use the same buyer-generated proposal id safely.""" + await store.put_draft( + proposal_id="shared", + account_id="acct_a", + recipes={}, + proposal_payload={"owner": "a"}, + ) + await store.put_draft( + proposal_id="shared", + account_id="acct_b", + recipes={}, + proposal_payload={"owner": "b"}, + ) + + record_a = await store.get("shared", expected_account_id="acct_a") + record_b = await store.get("shared", expected_account_id="acct_b") + assert record_a is not None and record_a.proposal_payload == {"owner": "a"} + assert record_b is not None and record_b.proposal_payload == {"owner": "b"} + # The API refuses an unscoped lookup instead of scanning across accounts. + with pytest.raises(TypeError, match="expected_account_id"): + await store.get("shared") # type: ignore[call-arg] + + @pytest.mark.asyncio async def test_get_by_media_buy_id_cross_tenant_returns_none( store: InMemoryProposalStore, @@ -570,7 +595,7 @@ async def test_media_buy_id_collision_across_tenants_does_not_clobber( async def test_discard_removes_record(store: InMemoryProposalStore) -> None: await store.put_draft(proposal_id="p1", account_id="acct_a", recipes={}, proposal_payload={}) await store.discard("p1", expected_account_id="acct_a") - assert await store.get("p1") is None + assert await store.get("p1", expected_account_id="acct_a") is None @pytest.mark.asyncio @@ -593,12 +618,12 @@ async def test_draft_evicted_after_ttl(fixed_clock: Any) -> None: ) await store.put_draft(proposal_id="p1", account_id="acct_a", recipes={}, proposal_payload={}) # Just after creation — record still present. - assert await store.get("p1") is not None + assert await store.get("p1", expected_account_id="acct_a") is not None fixed_clock.advance(timedelta(hours=23)) - assert await store.get("p1") is not None + assert await store.get("p1", expected_account_id="acct_a") is not None fixed_clock.advance(timedelta(hours=2)) # 25h total # Eviction runs on the next get. - assert await store.get("p1") is None + assert await store.get("p1", expected_account_id="acct_a") is None @pytest.mark.asyncio @@ -612,10 +637,10 @@ async def test_committed_evicted_past_grace(fixed_clock: Any) -> None: await store.commit("p1", expires_at=expires, proposal_payload={}, expected_account_id="acct_a") # 1h past commit — committed window not even reached. fixed_clock.advance(timedelta(hours=2)) - assert await store.get("p1") is not None + assert await store.get("p1", expected_account_id="acct_a") is not None # 8d past expires — beyond grace. fixed_clock.advance(timedelta(days=8)) - assert await store.get("p1") is None + assert await store.get("p1", expected_account_id="acct_a") is None @pytest.mark.asyncio @@ -636,7 +661,7 @@ async def test_refine_iteration_preserves_creation_time(fixed_clock: Any) -> Non ) fixed_clock.advance(timedelta(hours=5)) # 25h since FIRST put_draft # Even though we just refined, the original put_draft was 25h ago. - assert await store.get("p1") is None + assert await store.get("p1", expected_account_id="acct_a") is None # --------------------------------------------------------------------------- @@ -666,11 +691,11 @@ def put_draft(self, *, proposal_id, account_id, recipes, proposal_payload) -> No proposal_payload=proposal_payload, ) - def get(self, proposal_id, *, expected_account_id=None): + def get(self, proposal_id, *, expected_account_id): record = self._records.get(proposal_id) if record is None: return None - if expected_account_id is not None and record.account_id != expected_account_id: + if record.account_id != expected_account_id: return None return record diff --git a/tests/test_roster_store.py b/tests/test_roster_store.py index 95cf69aee..c2f25243c 100644 --- a/tests/test_roster_store.py +++ b/tests/test_roster_store.py @@ -1,12 +1,12 @@ """Tests for :func:`adcp.decisioning.create_roster_account_store`. Shape C ``AccountStore`` factory for publisher-curated rosters where the -adopter has a fixed allowlist of accounts. Pairs with +adopter has a fixed roster plus a principal/account authorization callback. Pairs with :class:`SingletonAccounts` (Shape derived) and :class:`ExplicitAccounts` (Shape explicit, loader-driven). -The roster IS the allowlist — auth-based filtering happens upstream of -this layer. Write paths (``upsert`` / ``sync_governance``) fail closed +Roster membership is not authorization. Write paths +(``upsert`` / ``sync_governance``) fail closed with ``PERMISSION_DENIED`` per-entry; the roster is read-only by design. """ @@ -45,6 +45,14 @@ def _make_roster() -> dict[str, Account]: } +_AUTH = AuthInfo(kind="derived", principal="agent_foo", credential=None) + + +def _allow_all(account: Account, auth: AuthInfo) -> bool: + del account + return auth.principal == "agent_foo" + + # --------------------------------------------------------------------------- # resolve # --------------------------------------------------------------------------- @@ -52,8 +60,8 @@ def _make_roster() -> dict[str, Account]: def test_resolve_hit_returns_account() -> None: """ref carrying a known ``account_id`` returns the roster entry.""" - store = create_roster_account_store(roster=_make_roster()) - result = asyncio.run(store.resolve(_by_id("acct_alpha"))) + store = create_roster_account_store(roster=_make_roster(), authorize=_allow_all) + result = asyncio.run(store.resolve(_by_id("acct_alpha"), auth_info=_AUTH)) assert result is not None assert result.id == "acct_alpha" assert result.name == "Alpha" @@ -62,8 +70,8 @@ def test_resolve_hit_returns_account() -> None: def test_resolve_miss_returns_none() -> None: """ref carrying an unknown ``account_id`` returns ``None`` — fall-through path the framework projects to ``ACCOUNT_NOT_FOUND``.""" - store = create_roster_account_store(roster=_make_roster()) - result = asyncio.run(store.resolve(_by_id("acct_unknown"))) + store = create_roster_account_store(roster=_make_roster(), authorize=_allow_all) + result = asyncio.run(store.resolve(_by_id("acct_unknown"), auth_info=_AUTH)) assert result is None @@ -71,8 +79,10 @@ def test_resolve_natural_key_returns_none() -> None: """``{brand, operator}``-shaped refs return ``None`` — publisher- curated rosters are queried by explicit id only. Adopters wanting natural-key resolution wrap ``resolve``.""" - store = create_roster_account_store(roster=_make_roster()) - result = asyncio.run(store.resolve(_by_natural_key("alpha.example.com", "alpha.example.com"))) + store = create_roster_account_store(roster=_make_roster(), authorize=_allow_all) + result = asyncio.run( + store.resolve(_by_natural_key("alpha.example.com", "alpha.example.com"), auth_info=_AUTH) + ) assert result is None @@ -81,8 +91,8 @@ def test_resolve_none_ref_returns_none() -> None: ``list_creative_formats``, ``preview_creative``) pass ``ref=None``; the helper returns ``None`` and adopters wrap to synthesize a publisher singleton when needed.""" - store = create_roster_account_store(roster=_make_roster()) - result = asyncio.run(store.resolve(None)) + store = create_roster_account_store(roster=_make_roster(), authorize=_allow_all) + result = asyncio.run(store.resolve(None, auth_info=_AUTH)) assert result is None @@ -90,22 +100,19 @@ def test_resolve_accepts_auth_info_kwarg() -> None: """The framework dispatcher calls ``accounts.resolve(ref_dict, auth_info=auth_info)`` — i.e. ``auth_info`` is a keyword argument on every dispatch path. Verify the roster store accepts that exact - call shape (and ignores ``auth_info`` because the roster IS the - allowlist).""" - store = create_roster_account_store(roster=_make_roster()) + call shape and uses it for the authorization callback.""" + store = create_roster_account_store(roster=_make_roster(), authorize=_allow_all) auth = AuthInfo(kind="signed_request", principal="agent_foo", scopes=["read"]) result = asyncio.run(store.resolve(_by_id("acct_alpha"), auth_info=auth)) assert result is not None assert result.id == "acct_alpha" -def test_resolve_positional_no_auth_info() -> None: - """Positional single-arg calls (no ``auth_info``) keep working — - matches the Protocol's ``auth_info=None`` default.""" - store = create_roster_account_store(roster=_make_roster()) +def test_resolve_without_auth_info_fails_closed() -> None: + """Possession of a known account id is not authorization.""" + store = create_roster_account_store(roster=_make_roster(), authorize=_allow_all) result = asyncio.run(store.resolve(_by_id("acct_beta"))) - assert result is not None - assert result.id == "acct_beta" + assert result is None def test_store_conforms_to_account_store_protocol() -> None: @@ -113,7 +120,7 @@ def test_store_conforms_to_account_store_protocol() -> None: boot-time platform validator calls ``isinstance(store, AccountStore)``. Any structural drift between the roster store's ``resolve`` signature and the Protocol breaks that check.""" - store = create_roster_account_store(roster=_make_roster()) + store = create_roster_account_store(roster=_make_roster(), authorize=_allow_all) assert isinstance(store, AccountStore) @@ -121,7 +128,7 @@ def test_resolution_literal_is_explicit() -> None: """Boot-time platform validation reads ``store.resolution`` to fail fast on misconfigured deployments. Roster stores are ``'explicit'`` — wire ref drives lookup.""" - store = create_roster_account_store(roster=_make_roster()) + store = create_roster_account_store(roster=_make_roster(), authorize=_allow_all) assert store.resolution == "explicit" @@ -131,11 +138,10 @@ def test_resolution_literal_is_explicit() -> None: def test_list_returns_full_roster() -> None: - """``list_accounts`` returns every roster entry. Auth-based - filtering is upstream — the roster IS the allowlist.""" + """An authorizer that grants both accounts returns the full roster.""" roster = _make_roster() - store = create_roster_account_store(roster=roster) - result = asyncio.run(store.list(ctx=ResolveContext())) + store = create_roster_account_store(roster=roster, authorize=_allow_all) + result = asyncio.run(store.list(ctx=ResolveContext(auth_info=_AUTH))) assert len(result) == 2 ids = {a.id for a in result} assert ids == {"acct_alpha", "acct_beta"} @@ -143,11 +149,59 @@ def test_list_returns_full_roster() -> None: def test_list_empty_roster() -> None: """Empty roster lists empty — not an error.""" - store = create_roster_account_store(roster={}) - result = asyncio.run(store.list(ctx=ResolveContext())) + store = create_roster_account_store(roster={}, authorize=_allow_all) + result = asyncio.run(store.list(ctx=ResolveContext(auth_info=_AUTH))) assert result == [] +def test_authorization_callback_filters_direct_lookup_and_list() -> None: + def authorize(account: Account, auth: AuthInfo) -> bool: + return account.id == "acct_alpha" and auth.principal == "agent_alpha" + + store = create_roster_account_store(roster=_make_roster(), authorize=authorize) + auth = AuthInfo(kind="derived", principal="agent_alpha", credential=None) + assert asyncio.run(store.resolve(_by_id("acct_alpha"), auth_info=auth)) is not None + assert asyncio.run(store.resolve(_by_id("acct_beta"), auth_info=auth)) is None + listed = asyncio.run(store.list(ctx=ResolveContext(auth_info=auth))) + assert [account.id for account in listed] == ["acct_alpha"] + + +def test_async_authorization_callback_can_grant() -> None: + async def authorize(account: Account, auth: AuthInfo) -> bool: + return account.id == "acct_alpha" and auth.principal == "agent_foo" + + store = create_roster_account_store(roster=_make_roster(), authorize=authorize) + result = asyncio.run(store.resolve(_by_id("acct_alpha"), auth_info=_AUTH)) + assert result is not None + + +def test_non_boolean_truthy_authorization_result_denies() -> None: + store = create_roster_account_store( + roster=_make_roster(), + authorize=lambda account, auth: [account.id, auth.principal], + ) + assert asyncio.run(store.resolve(_by_id("acct_alpha"), auth_info=_AUTH)) is None + + +def test_raising_authorization_callback_denies_and_logs(caplog: pytest.LogCaptureFixture) -> None: + def authorize(_account: Account, _auth: AuthInfo) -> bool: + raise RuntimeError("policy backend unavailable") + + store = create_roster_account_store(roster=_make_roster(), authorize=authorize) + + with caplog.at_level("ERROR", logger="adcp.decisioning.roster_store"): + result = asyncio.run(store.resolve(_by_id("acct_alpha"), auth_info=_AUTH)) + + assert result is None + assert "roster authorize callback raised; denying" in caplog.text + assert "policy backend unavailable" in caplog.text + + +def test_omitted_authorization_callback_fails_at_construction() -> None: + with pytest.raises(TypeError, match="authorize"): + create_roster_account_store(roster=_make_roster()) # type: ignore[call-arg] + + # --------------------------------------------------------------------------- # upsert — fail-closed PERMISSION_DENIED per entry # --------------------------------------------------------------------------- @@ -159,7 +213,7 @@ def test_upsert_denies_every_entry() -> None: ``failed`` row with ``PERMISSION_DENIED`` so the wire response surfaces the rejection per-entry instead of operation-level raising (which would fail the whole batch).""" - store = create_roster_account_store(roster=_make_roster()) + store = create_roster_account_store(roster=_make_roster(), authorize=_allow_all) refs = [ _by_natural_key("acme.com", "acme.com"), _by_natural_key("globex.com", "globex.com"), @@ -176,7 +230,7 @@ def test_upsert_denies_every_entry() -> None: def test_upsert_empty_refs_returns_empty() -> None: """An empty refs list returns an empty result list — not an error.""" - store = create_roster_account_store(roster=_make_roster()) + store = create_roster_account_store(roster=_make_roster(), authorize=_allow_all) rows = asyncio.run(store.upsert([], ctx=ResolveContext())) assert rows == [] @@ -189,7 +243,7 @@ def test_upsert_denies_by_id_refs_with_conformant_row_shape() -> None: shape conforms to :class:`SyncAccountsResultRow` (instance type + required fields populated, so the framework's wire projector won't crash on a missing field).""" - store = create_roster_account_store(roster=_make_roster()) + store = create_roster_account_store(roster=_make_roster(), authorize=_allow_all) rows = asyncio.run( store.upsert([_by_id("acct_alpha"), _by_id("acct_unknown")], ctx=ResolveContext()) ) @@ -210,7 +264,7 @@ def test_upsert_echoes_brand_operator_for_natural_key_refs() -> None: """``SyncAccountsResultRow.brand`` and ``operator`` are required on the wire. For natural-key refs we echo them back so the buyer can correlate the rejection to their request entry.""" - store = create_roster_account_store(roster=_make_roster()) + store = create_roster_account_store(roster=_make_roster(), authorize=_allow_all) rows = asyncio.run( store.upsert([_by_natural_key("acme.com", "acme.com")], ctx=ResolveContext()) ) @@ -228,7 +282,7 @@ def test_sync_governance_denies_every_entry() -> None: roster-backed store — the adopter doesn't model buyer-supplied governance bindings. Per-entry rejection (not operation-level) so a multi-account batch sees explicit rejection per row.""" - store = create_roster_account_store(roster=_make_roster()) + store = create_roster_account_store(roster=_make_roster(), authorize=_allow_all) entries = [ SyncGovernanceEntry( account=_by_id("acct_alpha"), @@ -251,7 +305,7 @@ def test_sync_governance_denies_every_entry() -> None: def test_sync_governance_echoes_account_ref() -> None: """``SyncGovernanceResultRow.account`` echoes the request ref so the buyer can correlate the rejection.""" - store = create_roster_account_store(roster=_make_roster()) + store = create_roster_account_store(roster=_make_roster(), authorize=_allow_all) ref = _by_id("acct_alpha") rows = asyncio.run( store.sync_governance( @@ -276,7 +330,7 @@ def test_construction_rejects_key_id_mismatch() -> None: "acct_beta": Account(id="acct_WRONG", name="Beta"), } with pytest.raises(ValueError) as exc_info: - create_roster_account_store(roster=bad_roster) + create_roster_account_store(roster=bad_roster, authorize=_allow_all) msg = str(exc_info.value) assert "acct_beta" in msg assert "acct_WRONG" in msg @@ -285,7 +339,7 @@ def test_construction_rejects_key_id_mismatch() -> None: def test_construction_accepts_empty_roster() -> None: """An empty roster is legal — adopter can ship an empty allowlist (every resolve misses, every list returns empty).""" - store = create_roster_account_store(roster={}) + store = create_roster_account_store(roster={}, authorize=_allow_all) assert store.resolution == "explicit" @@ -300,7 +354,7 @@ def test_external_mutation_does_not_leak_into_store() -> None: store's view — adopters who reuse the input dict for other purposes don't accidentally widen the allowlist.""" roster = _make_roster() - store = create_roster_account_store(roster=roster) + store = create_roster_account_store(roster=roster, authorize=_allow_all) # Buyer-side mutation: adopter clears their map after handing it # to the store. @@ -309,10 +363,10 @@ def test_external_mutation_does_not_leak_into_store() -> None: # Store still sees the original two entries; the injected attacker # entry is invisible. - listed = asyncio.run(store.list(ctx=ResolveContext())) + listed = asyncio.run(store.list(ctx=ResolveContext(auth_info=_AUTH))) ids = {a.id for a in listed} assert ids == {"acct_alpha", "acct_beta"} assert "acct_attacker" not in ids - attacker = asyncio.run(store.resolve(_by_id("acct_attacker"))) + attacker = asyncio.run(store.resolve(_by_id("acct_attacker"), auth_info=_AUTH)) assert attacker is None diff --git a/tests/test_tenant_store.py b/tests/test_tenant_store.py index 0f323723f..0823ec7c3 100644 --- a/tests/test_tenant_store.py +++ b/tests/test_tenant_store.py @@ -3,8 +3,8 @@ Mirrors the security semantics of the JS-side ``createTenantStore`` at ``packages/sdk/src/server/decisioning/tenant-store.ts``: cross-tenant -entries on ``upsert`` / ``sync_governance`` are rejected with -``PERMISSION_DENIED`` BEFORE reaching adopter callbacks. Fail-closed +and unknown entries on ``upsert`` / ``sync_governance`` collapse to +``ACCOUNT_NOT_FOUND`` before reaching adopter callbacks. Fail-closed when ``resolve_from_auth`` returns ``None``. The gate methods (``upsert``, ``sync_governance``) are defined on the @@ -301,12 +301,11 @@ def test_cross_tenant_entry_rejected_before_adopter_code(self) -> None: assert rows[0].action == "failed" assert rows[0].status == "rejected" assert rows[0].errors is not None - assert rows[0].errors[0]["code"] == "PERMISSION_DENIED" + assert rows[0].errors[0]["code"] == "ACCOUNT_NOT_FOUND" + assert rows[0].errors[0]["recovery"] == "terminal" def test_unknown_ref_rejected_with_account_not_found(self) -> None: - """ACCOUNT_NOT_FOUND is the right code for "ref points - nowhere" — distinct from PERMISSION_DENIED ("ref valid but - you're not authorized").""" + """Unknown and unauthorized refs share the existence-hiding result.""" store, writes = self._build_with_recorder() rows = _run(store.upsert([_ref("unknown.example")], _ctx("buyer@pinnacle"))) assert writes == [] @@ -338,8 +337,7 @@ def test_fail_closed_unauthenticated_rejects_every_entry(self) -> None: assert rows[0].errors[0]["code"] == "PERMISSION_DENIED" def test_mixed_batch_partitions_correctly(self) -> None: - """Pass / cross-tenant / unknown — three distinct outcomes, - only the in-tenant entry reaches adopter code.""" + """Only the in-tenant entry reaches adopter code; both probes fail alike.""" store, writes = self._build_with_recorder() rows = _run( store.upsert( @@ -354,7 +352,7 @@ def test_mixed_batch_partitions_correctly(self) -> None: assert len(writes) == 1, "only the in-tenant entry should reach upsert_row" assert rows[0].action == "created" assert rows[1].errors is not None - assert rows[1].errors[0]["code"] == "PERMISSION_DENIED" + assert rows[1].errors[0]["code"] == "ACCOUNT_NOT_FOUND" assert rows[2].errors is not None assert rows[2].errors[0]["code"] == "ACCOUNT_NOT_FOUND" @@ -570,7 +568,7 @@ def test_in_tenant_entry_passes_through(self) -> None: assert len(writes) == 1 assert rows[0].status == "synced" - def test_cross_tenant_entry_rejected(self) -> None: + def test_cross_tenant_entry_hidden_as_account_not_found(self) -> None: store, writes = self._build_with_recorder() entries = [ SyncGovernanceEntry( @@ -582,7 +580,36 @@ def test_cross_tenant_entry_rejected(self) -> None: assert writes == [] assert rows[0].status == "failed" assert rows[0].errors is not None - assert rows[0].errors[0]["code"] == "PERMISSION_DENIED" + assert rows[0].errors[0]["code"] == "ACCOUNT_NOT_FOUND" + + def test_unknown_and_cross_tenant_entries_are_indistinguishable(self) -> None: + """A caller cannot enumerate valid operators from governance errors.""" + cross_store, cross_writes = self._build_with_recorder() + unknown_store = create_tenant_store( + resolve_by_ref=lambda ref, ctx: None, + resolve_from_auth=_resolve_from_auth, + tenant_id=_account_tenant_id, + tenant_to_account=_tenant_to_account, + ) + entries = [ + SyncGovernanceEntry( + account=_ref("pinnacle.example"), + governance_agents=[], + ) + ] + + cross_rows = _run(cross_store.sync_governance(entries, _ctx("buyer@meridian"))) + unknown_rows = _run(unknown_store.sync_governance(entries, _ctx("buyer@meridian"))) + + assert cross_writes == [] + assert cross_rows[0].errors == unknown_rows[0].errors + assert cross_rows[0].errors == [ + { + "code": "ACCOUNT_NOT_FOUND", + "message": "Unknown operator: pinnacle.example", + "recovery": "terminal", + } + ] def test_fail_closed_no_auth_rejects_every_entry(self) -> None: store, writes = self._build_with_recorder() diff --git a/tests/test_upstream_for.py b/tests/test_upstream_for.py index 8e23ed3d8..b6cf1e326 100644 --- a/tests/test_upstream_for.py +++ b/tests/test_upstream_for.py @@ -24,7 +24,9 @@ from __future__ import annotations +import asyncio from typing import Any +from unittest.mock import AsyncMock import pytest @@ -235,6 +237,95 @@ def test_repeated_call_same_url_returns_same_client_instance() -> None: assert client_1 is client_2 +def test_repeated_default_no_auth_reuses_client() -> None: + """Omitting auth must not allocate a fresh cache identity per request.""" + platform = _Platform() + client_1 = platform.upstream_for(_ctx(mode="live")) + client_2 = platform.upstream_for(_ctx(mode="live")) + assert client_1 is client_2 + + +def test_default_headers_are_part_of_cache_identity() -> None: + """Tenant routing headers cannot bleed through a shared cached client.""" + platform = _Platform() + auth = StaticBearer(token="shared") + client_a = platform.upstream_for( + _ctx(mode="live"), auth=auth, default_headers={"X-Tenant": "tenant-a"} + ) + client_b = platform.upstream_for( + _ctx(mode="live"), auth=auth, default_headers={"X-Tenant": "tenant-b"} + ) + assert client_a is not client_b + assert client_a._default_headers == {"X-Tenant": "tenant-a"} + assert client_b._default_headers == {"X-Tenant": "tenant-b"} + + +def test_transport_options_are_part_of_cache_identity() -> None: + platform = _Platform() + auth = StaticBearer(token="shared") + default = platform.upstream_for(_ctx(mode="live"), auth=auth) + different_timeout = platform.upstream_for(_ctx(mode="live"), auth=auth, timeout=5.0) + different_404 = platform.upstream_for(_ctx(mode="live"), auth=auth, treat_404_as_none=False) + assert len({id(default), id(different_timeout), id(different_404)}) == 3 + + +@pytest.mark.asyncio +async def test_bounded_pool_retires_evicted_client_until_shutdown() -> None: + platform = _Platform() + platform.upstream_client_cache_size = 1 + first = platform.upstream_for(_ctx(mode="live"), auth=StaticBearer(token="first")) + first.aclose = AsyncMock() # type: ignore[method-assign] + + second = platform.upstream_for(_ctx(mode="live"), auth=StaticBearer(token="second")) + second.aclose = AsyncMock() # type: ignore[method-assign] + + first.aclose.assert_not_awaited() + assert len(platform._upstream_client_pool._cache) == 1 + assert platform._upstream_client_pool._retired == [first] + + await platform.aclose_upstream_clients() + first.aclose.assert_awaited_once() + second.aclose.assert_awaited_once() + + +def test_sync_eviction_drains_cached_and_retired_clients() -> None: + platform = _Platform() + platform.upstream_client_cache_size = 1 + first = platform.upstream_for(_ctx(mode="live"), auth=StaticBearer(token="first")) + second = platform.upstream_for(_ctx(mode="live"), auth=StaticBearer(token="second")) + first.aclose = AsyncMock() # type: ignore[method-assign] + second.aclose = AsyncMock() # type: ignore[method-assign] + + asyncio.run(platform.aclose_upstream_clients()) + + first.aclose.assert_awaited_once() + second.aclose.assert_awaited_once() + + +def test_zero_upstream_client_cache_size_fails_closed() -> None: + platform = _Platform() + platform.upstream_client_cache_size = 0 + + with pytest.raises(ValueError, match="max_size must be at least 1"): + platform.upstream_for(_ctx(mode="live")) + + +def test_upstream_client_pool_updates_lru_recency_on_hit() -> None: + platform = _Platform() + platform.upstream_client_cache_size = 2 + auth_a = StaticBearer(token="a") + auth_b = StaticBearer(token="b") + auth_c = StaticBearer(token="c") + client_a = platform.upstream_for(_ctx(mode="live"), auth=auth_a) + client_b = platform.upstream_for(_ctx(mode="live"), auth=auth_b) + + assert platform.upstream_for(_ctx(mode="live"), auth=auth_a) is client_a + client_c = platform.upstream_for(_ctx(mode="live"), auth=auth_c) + + assert list(platform._upstream_client_pool._cache.values()) == [client_a, client_c] + assert platform._upstream_client_pool._retired == [client_b] + + def test_distinct_auth_strategies_get_distinct_clients() -> None: """Different auth instances ⇒ different clients. The auth is injected at construction; the framework can't swap it on a cached diff --git a/tests/test_upstream_helpers.py b/tests/test_upstream_helpers.py index 1ecb21cfb..ab6fd6aad 100644 --- a/tests/test_upstream_helpers.py +++ b/tests/test_upstream_helpers.py @@ -272,6 +272,7 @@ async def test_get_404_raises_media_buy_not_found_when_disabled() -> None: with pytest.raises(AdcpError) as exc_info: await client.get("/items/missing") assert exc_info.value.code == "MEDIA_BUY_NOT_FOUND" + assert exc_info.value.recovery == "correctable" await client.aclose() @@ -282,6 +283,7 @@ async def test_get_404_with_custom_not_found_code() -> None: with pytest.raises(AdcpError) as exc_info: await client.get("/creatives/x", not_found_code="CREATIVE_NOT_FOUND") assert exc_info.value.code == "CREATIVE_NOT_FOUND" + assert exc_info.value.recovery == "correctable" await client.aclose() @@ -293,6 +295,7 @@ async def test_post_404_always_raises() -> None: with pytest.raises(AdcpError) as exc_info: await client.post("/items", json={}) assert exc_info.value.code == "MEDIA_BUY_NOT_FOUND" + assert exc_info.value.recovery == "correctable" await client.aclose() @@ -308,7 +311,7 @@ async def test_401_raises_auth_required() -> None: with pytest.raises(AdcpError) as exc_info: await client.get("/x") assert exc_info.value.code == "AUTH_REQUIRED" - assert exc_info.value.recovery == "terminal" + assert exc_info.value.recovery == "correctable" await client.aclose() @@ -319,6 +322,7 @@ async def test_403_raises_permission_denied() -> None: with pytest.raises(AdcpError) as exc_info: await client.get("/x") assert exc_info.value.code == "PERMISSION_DENIED" + assert exc_info.value.recovery == "correctable" await client.aclose() @@ -344,6 +348,21 @@ async def test_500_raises_service_unavailable() -> None: await client.aclose() +@respx.mock +async def test_error_response_body_is_not_exposed() -> None: + secret = "upstream-secret-token" + respx.get(f"{BASE}/x").mock( + return_value=httpx.Response(500, text=f"database failed Authorization=Bearer {secret}") + ) + client = create_upstream_http_client(BASE) + with pytest.raises(AdcpError) as exc_info: + await client.get("/x") + + assert secret not in str(exc_info.value) + assert "database failed" not in str(exc_info.value) + await client.aclose() + + @respx.mock async def test_400_raises_invalid_request_correctable() -> None: respx.get(f"{BASE}/x").mock(return_value=httpx.Response(400, text="bad")) @@ -351,7 +370,7 @@ async def test_400_raises_invalid_request_correctable() -> None: with pytest.raises(AdcpError) as exc_info: await client.get("/x") assert exc_info.value.code == "INVALID_REQUEST" - assert exc_info.value.recovery == "retry_with_changes" + assert exc_info.value.recovery == "correctable" await client.aclose()