Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fix(identity): enforce tenant and credential isolation #1001
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Uh oh!
There was an error while loading. Please reload this page.
fix(identity): enforce tenant and credential isolation #1001
Changes from all commits
27a7998File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two defects on this line.
The guard raises from a hook no transport projects.
serve.py:2656callscontext_factory(meta)above thetrywhoseexcept ADCPErrorat:2682builds the conformant envelope, anda2a_server.py:302has the same shape. Verified at head:{"result":{"content":[{"text":"Error executing tool get_products: AdcpError[PERMISSION_DENIED / terminal]: Bearer credential is not valid for this tenant.","type":"text"}],"isError":true}}No
adcp_error, nocode, norecovery, and the exception repr is what lands in the tool text — for a condition any holder of a valid token triggers by changing the request host. On A2A the same raise escapesexecute()and becomes a JSON-RPC transport error: one rule, two non-conformant and mutually different envelopes.The condition fails open.
ctx.tenant_id is not None and ...means deployments that leavePrincipal.tenant_idunset — documented as supported atsrc/adcp/server/auth.py:148-152— still silently rebind the credential to whatever tenant the Host names, which is what the comment above says it prevents. Fail closed when a host tenant was resolved and the token carries none.Root: the rule is framework-shaped and the framework neither enforces nor offers it. Both identities are visible at one point (the auth ContextVar holds the token's tenant,
tenant_router.current_tenant()the host's), and the framework prints the vulnerable recipe twice —tenant_router.py:55-61andtenant_registry.py:770-776. Do the comparison inauth_context_factory(or ship asubdomain_auth_context_factory), update both docstrings, and delete this check.test_bearer_context_rejects_cross_tenant_rebindingcalls the factory directly and asserts onlyexcinfo.value.code, so the wire shape and both allow paths (tenant_id is Nonemust still pin;tenant_id == tenant.idmust pin, not reject) are uncovered.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two spec problems on this remap. AdCP 3.1.8 marks
AUTH_REQUIREDDeprecated —enumDescriptions.AUTH_REQUIRED: "useAUTH_MISSING(no credentials presented) orAUTH_INVALID(credentials presented and rejected)" — and the condition here is exactly "no credentials presented", so the code isAUTH_MISSING(which is in the pinned enum). AndenumMetadatagivesAUTH_REQUIRED: correctable, so therecovery="terminal"three lines down is wrong either way.The remap is also broader than it looks: the old code returned
ACCOUNT_NOT_FOUND/correctable/field="account.account_id"with a message telling the buyer what to send, and the explicit-account_idarm did not require a principal at all. Every unauthenticated resolve now gets a terminal code. Nothing asserts either half — deleting this whole block leavespytest examples/v3_reference_seller/tests/at59 passed, 2 deselected. EmitAUTH_MISSINGwithrecovery="correctable", citeschemas/cache/3.1/enums/error-code.json@3.1.8at the raise site, and addstore.resolve({"account_id": "x"}, None)plus the brand-shaped-ref case assertingcodeandrecovery.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This predicate is the new cross-buyer isolation guarantee, and no test observes it.
test_account_store_explicit_id_is_bound_to_authenticated_buyerstubssession.executewithAsyncMock(side_effect=[buyer_result, missing_account_result]), so the second query returnsNoneregardless of itsWHEREclause — what it proves is thatresolvenow issues two queries, not the ownership binding.Verified: deleting this line and running
pytest examples/v3_reference_seller/tests/gives59 passed, 2 deselected. The cross-buyer read path reopens with a green suite, and the storyboard job cannot substitute because it drives only correctly-owned accounts. The sibling predicate on the brand-shaped path at:347has the same exposure.The shape to copy is already in this diff:
test_account_store_upsert_cannot_overwrite_another_buyers_accountuses a realAccountRowand assertsforeign.buyer_agent_id == "ba_other"afterwards. Use a real sqlite/asyncpg session with twoAccountRows in tenantt_acme, one owned byba_callerand one byba_other: resolvingba_other'saccount_idasba_callermust beACCOUNT_NOT_FOUND, andba_caller's own must still resolve.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This raises inside the
for incoming in refsloop, insideasync with session.begin(), so a batch where any single entry names another buyer's account aborts the call and rolls back the entries that already succeeded. Verified with a two-entry batch (entry 1 new, entry 2 foreign):schemas/cache/3.1/account/sync-accounts-response.json@ 3.1.8 reserves the operation-level channel for "complete failure"; the per-entry channel isaccounts[i].action="failed"+status="rejected"+errors[]. It is also inconsistent with what this same PR does one layer up —tenant_store.upsertemits a per-entryACCOUNT_NOT_FOUNDrow for the identical cross-boundary condition.Append a
SyncAccountsResultRow(action="failed", status="rejected", errors=[...])andcontinue.field="accounts"is also unindexed where the spec's convention is an exact path (FIELD_NOT_PERMITTED: "error.fieldMUST identify the exact offending field path (e.g.,packages[0].budget)") — point it ataccounts[<i>]. The new test uses a single-entry batch, so add the mixed-batch case asserting the clean entry still lands.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ownership check is bypassable with the repo's own seed data.
seed.py:117-142seedsa_acme_1(buyer agentba_acme_signed) anda_acme_2(ba_acme_bearer) with the sameext = {"network_code": "net_premium_us", "advertiser_id": "adv_volta_motors"}— two buyer agents, one advertiser id, so the comparison passes straight across the account boundary. The gate guardsupdate_media_buy(:1394),get_media_buy_delivery(:1685, reads another buyer's spend and impressions) andprovide_performance_feedback(:1912, writes conversions onto another buyer's campaign).test_update_media_buy_rejects_foreign_advertiser_orderonly exercises a different advertiser id.The stronger identity already exists in this class:
create_media_buyrecords_account_scope(ctx)on the buy state at:1110-1112, and the creative maps were correctly re-keyed to(account_scope, id). Pick one identity —buyer_agent_idis what the DB enforces — and check that.Two more edges here. An absent
advertiser_idon the upstreamGET /orders/{id}is indistinguishable from a foreign order, so upstream schema drift becomes a silent totalMEDIA_BUY_NOT_FOUNDoutage that looks spec-correct; an absent field should be an operator-visibleSERVICE_UNAVAILABLE. And the comment on the next line claims foreign and nonexistent ids are indistinguishable, but the envelopes differ:3.1.8's
*_NOT_FOUNDuniform-response rule (schemas/cache/3.1/enums/error-code.json@3.1.8) requires the response shape itself not be the oracle. Route both arms through one constructor and assertforeign.to_wire() == missing.to_wire(); the three new tests assert onlyexcinfo.value.code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
recovery="terminal"contradicts the pinned spec:schemas/cache/3.1/enums/error-code.json#enumMetadata@ 3.1.8 givesMEDIA_BUY_NOT_FOUND: correctable, andcore/error.jsoncallsrecoverythe normative carrier of recovery semantics. A conforming buyer classifying on it treats a fixable-and-resend condition as escalate-to-human.The repo already owns the abstraction:
src/adcp/decisioning/errors.pybinds each code to its enumMetadata recovery (MediaBuyNotFoundError → correctable, verified) and its module docstring says "Recovery values are normative — they MUST match theenumMetadatablock." RaiseMediaBuyNotFoundErrorinstead of the base class here and at the sibling new sites (platform.py:295,app.py:117).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The creative maps were re-keyed to
(account_scope, id);_buy_statekeeps the bareorder_idkey withaccount_scopewritten into the value, and that field is checked at one of eight read sites (:1625).:1331,:1422,:1499,:1640,:1704,:1791and:1819read or mutate_buy_state[order_id]/_buy_revisions[order_id]with no scope check. Today they are safe only because upstream order ids are globally unique in the mock and the callers are separately gated by_get_owned_order— which is the "safe only if" invariant that should live in the key rather than in a reviewer's head.Also:
setdefault(order_id, ...)["account_scope"] = ...silently rebinds an existing entry's owner instead of refusing, and_account_scopeat:871builds the scope fromctx.account.metadata.get("tenant_id") or "", so anAccountconstructed without that metadata key yields a tenant-less scope instead of failing.Key
_buy_stateand_buy_revisionsby(owner_key, order_id)like the creative maps so no reader can forget the check, and derive the key once from a typed metadata model —Accountis generic inTMeta, so aTypedDictwith requiredtenant_id/account_id/advertiser_id/network_code/buyer_agent_idturns roughly tenmetadata["..."]/.get(...) or ""sites into checked attribute access. Neither the write here nor the gate at:1625is exercised: replacing the:1626comparison with== "NEVER_MATCHES"leaves59 passed, 2 deselected.Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.