From 4d17046391a21b7ad3c66d5bbfb25c7e2d5cd141 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Mon, 4 May 2026 10:03:48 -0400 Subject: [PATCH 1/2] fix(auth): populate ctx.auth_principal from bearer ContextVar MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For bearer-token adopters using BearerTokenAuthMiddleware, RequestContext.auth_principal was always None because the middleware never constructs an AuthInfo — the principal lives in the adcp.server.auth.current_principal ContextVar that auth_context_factory reads. Adopters trying to read "who's calling?" inside a decisioning handler had to reach into the framework-private ContextVar themselves, or misread ctx.caller_identity (which the framework dispatch helper mutates downstream into a composite cache scope key for idempotency, e.g. core.stores.accounts.SalesagentAccountStore:wonderstruck:default). _build_request_context now falls back to current_principal.get() when no AuthInfo is present (or its principal is None), so ctx.auth_principal is the right read on both signed-request and bearer flows. Updates context.py + dispatch.py docstrings to document the two sources, the example to comment on the correct field to read, and handler-authoring.md with a section disambiguating auth_principal vs caller_identity. User-visible behavior shift: bearer-flow callers that previously saw ctx.auth_principal == None will now see the bearer principal. Adopter code that gated on `if ctx.auth_principal is None: ...` for bearer flows will silently change behavior — this is intended (the previous None was the bug) but worth surfacing in the changelog. Closes #571 Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/handler-authoring.md | 24 +++++++++++ examples/mcp_with_auth_middleware.py | 9 +++++ src/adcp/decisioning/context.py | 59 ++++++++++++++++++---------- src/adcp/decisioning/dispatch.py | 36 +++++++++++++++-- tests/test_decisioning_dispatch.py | 41 ++++++++++++++++++- 5 files changed, 144 insertions(+), 25 deletions(-) diff --git a/docs/handler-authoring.md b/docs/handler-authoring.md index b5958ec0d..acd97de4e 100644 --- a/docs/handler-authoring.md +++ b/docs/handler-authoring.md @@ -295,6 +295,30 @@ and leak the presence of an auth path to attackers). Full worked example: `examples/mcp_with_auth_middleware.py`. Integration test proving the composition: `tests/test_mcp_middleware_composition.py`. +#### Reading "who's calling?" — `auth_principal` vs `caller_identity` + +The two identity-shaped fields adopters see have distinct purposes and +the wrong read silently misroutes authorization decisions. + +- **`ctx.auth_principal`** — the verified caller's identity label. + Read this for per-principal ACLs ("can principal X mutate this + buy?"). Sourced from `AuthInfo.principal` on signed-request flows + and from the `current_principal` ContextVar on bearer flows. + `None` for unauthenticated dev fixtures. +- **`ctx.caller_identity`** — a cache scope key, not a principal. + At the transport layer (`ToolContext.caller_identity`) it's the + bare principal; the framework dispatch helper mutates it into a + composite key (`.:`) + before the handler sees a decisioning `RequestContext`. The + idempotency middleware reads the composite form. Treat as opaque + inside dispatch handlers — log or forward, but don't parse, + compare, or rewrite. + +Skill-middleware and `context_factory` code (which run at the +transport layer, before dispatch hydration) still read +`context.caller_identity` for legitimate cache / rate-limit keying; +the composite mutation happens later, in `_build_request_context`. + #### Pattern 2a — custom middleware (when the shipped one doesn't fit) Subclass `BearerTokenAuthMiddleware` to tighten the discovery bypass, diff --git a/examples/mcp_with_auth_middleware.py b/examples/mcp_with_auth_middleware.py index adb290200..91c32094e 100644 --- a/examples/mcp_with_auth_middleware.py +++ b/examples/mcp_with_auth_middleware.py @@ -63,6 +63,15 @@ async def get_adcp_capabilities( return capabilities_response(["media_buy"]) async def get_products(self, params: Any, context: ToolContext | None = None) -> dict[str, Any]: + # On the dispatch path the handler receives a decisioning + # ``RequestContext``; read ``ctx.auth_principal`` (not + # ``ctx.caller_identity``) for "who's calling?". The + # framework mutates ``caller_identity`` downstream into a + # composite cache scope key for idempotency — it is not a + # principal label by the time a handler sees it. On bearer + # flows like this one ``auth_principal`` is sourced from the + # :data:`adcp.server.auth.current_principal` ContextVar that + # the middleware populates. tenant = context.tenant_id if context is not None else None return products_response(_products_for_tenant(tenant)) diff --git a/src/adcp/decisioning/context.py b/src/adcp/decisioning/context.py index 21b618512..16e5b63ce 100644 --- a/src/adcp/decisioning/context.py +++ b/src/adcp/decisioning/context.py @@ -475,23 +475,38 @@ class RequestContext(ToolContext, Generic[TMeta]): DB queries, and stamp audit logs. ``auth_principal`` — "who's calling?" - The verified caller's identity label. The string varies by - auth shape: ``agent_url`` for AdCP v3 signed-request agents - (the documented convention; the SDK's signed-request adapter - wrappers ship in 4.5.0), OAuth subject claim for bearer - flows, mTLS subject for client-cert flows. Read it for - per-principal ACLs *within* an account ("can principal X - mutate this buy?"). + The verified caller's identity label and the typed read for + adopter authorization checks. Populated from two sources + depending on the auth flow: + + * **Signed-request flows** — sourced from + :class:`AuthInfo.principal` (``agent_url`` for AdCP v3 + signed-request agents per spec). + * **Bearer-token flows** — sourced from the + :data:`adcp.server.auth.current_principal` ContextVar that + :class:`BearerTokenAuthMiddleware` populates + (``Principal.caller_identity`` from the validator). + + Read it for per-principal ACLs *within* an account ("can + principal X mutate this buy?"). ``None`` for unauthenticated + dev fixtures. ``caller_identity`` — "what's the cache scope key?" - Composite framework-set key - (``.:``) used by - the idempotency middleware to scope the replay cache. - Treat as opaque. Adopter code may log or forward it - (rate-limiting, audit) but should not parse, compare, or - rewrite it — the format is framework-internal and any - adopter assumption about its shape will break when the - scope-key composition changes. + Starts as the bare principal at the transport layer + (:class:`ToolContext.caller_identity`), then the framework + dispatch helper mutates it into the composite scope key + (``.:``) before + the handler sees the :class:`RequestContext`. The + idempotency middleware reads the composite form to scope + the replay cache. + + **Do not read this for identity decisions** — by the time a + handler observes the field it's a cache key, not a + principal label. Use ``auth_principal`` for "who's calling?" + and treat ``caller_identity`` as opaque (log / forward only; + don't parse, compare, or rewrite). The composite format is + framework-internal and any adopter assumption about its + shape will break when the scope-key composition changes. ``tenant_id`` — "which transport tenant?" Inherited from :class:`ToolContext`; set by the transport @@ -519,11 +534,15 @@ class RequestContext(ToolContext, Generic[TMeta]): ``NotImplementedError`` on every call. v6.1 wires the backing fetchers. :param auth_principal: Typed convenience field carrying the - verified principal label (sourced from - :class:`AuthInfo.principal` when present). Distinct from - ``account.id`` (which the framework's idempotency middleware - uses for cache scope) — middleware reading "who authenticated - this request" gets a load-bearing field name. + verified principal label. Sourced from + :class:`AuthInfo.principal` on signed-request flows and from + the :data:`adcp.server.auth.current_principal` ContextVar + on bearer-token flows (the framework's + :class:`BearerTokenAuthMiddleware` populates the + ContextVar; the dispatch helper reads it when ``auth_info`` + is absent). The right read for "who's calling?" — distinct + from ``caller_identity``, which the framework mutates into + a composite cache scope key for idempotency. """ # Default factories so ``RequestContext()`` works in tests; in diff --git a/src/adcp/decisioning/dispatch.py b/src/adcp/decisioning/dispatch.py index 70afbe161..37332fa61 100644 --- a/src/adcp/decisioning/dispatch.py +++ b/src/adcp/decisioning/dispatch.py @@ -1006,8 +1006,15 @@ def _build_request_context( composite key. :param account: Resolved account from the platform's :class:`AccountStore.resolve`. - :param auth_info: Optional verified principal info — when present, - ``auth_principal`` is populated from ``auth_info.principal``. + :param auth_info: Optional verified principal info — when present + and carrying a non-``None`` principal, ``auth_principal`` is + populated from ``auth_info.principal``. Otherwise the helper + falls back to :data:`adcp.server.auth.current_principal` — + the ContextVar :class:`BearerTokenAuthMiddleware` populates — + so bearer-flow callers get a typed read for "who's calling?" + without reaching into framework-private state. Returns + ``None`` outside both flows (no-op for unauthenticated dev + fixtures). :param store: The AccountStore that produced ``account``. Required for the production cache-isolation guarantee; the dispatch adapter always supplies it. Test fixtures may pass ``None`` @@ -1024,7 +1031,30 @@ def _build_request_context( from adcp.decisioning.context import RequestContext from adcp.decisioning.resolve import _NotYetWiredResolver - auth_principal = auth_info.principal if auth_info is not None else None + # ``auth_principal`` is the typed "who's calling?" read for + # adopter handlers. Two sources populate it: + # + # * Signed-request flows hydrate ``AuthInfo`` upstream and the + # adapter passes it as ``auth_info``; ``auth_info.principal`` + # carries the verified caller label. + # * Bearer-token flows (:class:`BearerTokenAuthMiddleware`) never + # construct an ``AuthInfo``; they stash the principal in the + # :data:`adcp.server.auth.current_principal` ContextVar instead. + # Read it as the fallback so bearer adopters can gate on + # ``ctx.auth_principal`` without reaching into the framework- + # private ContextVar themselves. ``.get()`` returns ``None`` + # outside a bearer flow — that's the desired no-op for non- + # bearer callers (signed-request without ``AuthInfo``, + # unauthenticated dev fixtures). + # + # Local import sidesteps a server→decisioning import cycle. + from adcp.server.auth import current_principal as _current_principal + + auth_principal = ( + auth_info.principal + if auth_info is not None and auth_info.principal is not None + else _current_principal.get() + ) # ctx_metadata credential gate — fail-closed before any platform # method sees the metadata. Buyers can populate ``context`` diff --git a/tests/test_decisioning_dispatch.py b/tests/test_decisioning_dispatch.py index 757e19323..057ffccee 100644 --- a/tests/test_decisioning_dispatch.py +++ b/tests/test_decisioning_dispatch.py @@ -425,8 +425,8 @@ def test_build_request_context_uses_composite_key_when_store_supplied() -> None: def test_build_request_context_with_no_auth() -> None: - """Unauthenticated dev path (singleton fixtures): auth_principal - is None, auth_info is None.""" + """Unauthenticated dev path (singleton fixtures): no AuthInfo and + no bearer ContextVar populated → auth_principal is None.""" tool_ctx = ToolContext() account: Account[Any] = Account(id="dev") ctx = _build_request_context(tool_ctx, account, None) @@ -434,6 +434,43 @@ def test_build_request_context_with_no_auth() -> None: assert ctx.auth_principal is None +def test_build_request_context_falls_back_to_bearer_context_var() -> None: + """Bearer-flow callers populate :data:`adcp.server.auth.current_principal` + via :class:`BearerTokenAuthMiddleware`; the dispatch helper must read + the ContextVar when no ``AuthInfo`` is provided so adopters can read + ``ctx.auth_principal`` instead of reaching into framework-private + state. Regression test for issue #571.""" + from adcp.server.auth import current_principal + + tool_ctx = ToolContext() + account: Account[Any] = Account(id="acct") + token = current_principal.set("principal-from-bearer") + try: + ctx = _build_request_context(tool_ctx, account, None) + finally: + current_principal.reset(token) + assert ctx.auth_info is None + assert ctx.auth_principal == "principal-from-bearer" + + +def test_build_request_context_auth_info_takes_precedence_over_bearer_var() -> None: + """When both ``AuthInfo`` and the bearer ContextVar are populated + (e.g. a custom middleware stack that hydrates both), the explicit + ``AuthInfo.principal`` wins. Bearer fallback is strictly the + "no AuthInfo" path.""" + from adcp.server.auth import current_principal + + tool_ctx = ToolContext() + account: Account[Any] = Account(id="acct") + auth = AuthInfo(kind="signed_request", principal="signed-buyer", key_id="kid") + token = current_principal.set("principal-from-bearer") + try: + ctx = _build_request_context(tool_ctx, account, auth) + finally: + current_principal.reset(token) + assert ctx.auth_principal == "signed-buyer" + + def test_build_request_context_supplies_stubs_when_no_state_resolver() -> None: """Default state/resolve are the v6.0 stubs — adopter call sites work without explicit wiring.""" From 073beece8adf53915ec676c3a4538dcebe66c361 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Mon, 4 May 2026 10:16:26 -0400 Subject: [PATCH 2/2] docs(dispatch): correct local-import justification comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous comment claimed the local import of adcp.server.auth.current_principal sidestepped a server→decisioning import cycle, but adcp.server.auth does not import from adcp.decisioning, so there is no cycle. Reword the justification to state the actual reason: keeping the layering local without forcing a top-level dep on adcp.server.auth. Comment-only change; no behavior or import structure changes. --- src/adcp/decisioning/dispatch.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/adcp/decisioning/dispatch.py b/src/adcp/decisioning/dispatch.py index 37332fa61..e36a613aa 100644 --- a/src/adcp/decisioning/dispatch.py +++ b/src/adcp/decisioning/dispatch.py @@ -1047,7 +1047,8 @@ def _build_request_context( # bearer callers (signed-request without ``AuthInfo``, # unauthenticated dev fixtures). # - # Local import sidesteps a server→decisioning import cycle. + # Local import keeps the layering local — read the bearer ContextVar + # without forcing a top-level dep on adcp.server.auth. from adcp.server.auth import current_principal as _current_principal auth_principal = (