Skip to content

RFC: the admin plane's authorization, under the AuthZ design #2256

Description

@KonstantinMirin

Parent: #2232, "RFC: AuthZ design".

This issue is subordinate to that one and proposes no second architecture. It adopts §5.1's
component decomposition, §5.2's Decision vocabulary, §5.3's two decision points, and §5.4's
reason-to-wire mapping, all by reference. Read those sections rather than a paraphrase of them
here.

#2232 §5.3 already seats the admin plane:
its policy enforcement point has three seats, and one of them is "the admin plane's decorators".
Its "Still open, tracked" table then says the admin plane "proceeds on its own track", and asks
one thing of it — "that its decisions reach the same decision vocabulary". That sentence is this
issue's charter.

Scope: src/admin/, plus the route handlers that src/adapters/ and src/services/
register onto the admin app. Not the buyer plane.

Proposal: consolidate the admin plane's ten authorization mechanisms onto the one seat
#2232 §5.3 names, and have that seat return a
Decision.

The measured asymmetry

Two planes serve the same database under the same tenant model. The buyer plane decides
authorization once; the admin plane decides it ten ways.

Buyer plane Admin plane
Authorization seams 1 10
Tenant scoping Carried by the type @require_tenant_access, 148 applications across 20 files
Handlers without authorization 0, and a type error to add one 33 of 179 tenant-scoped URL rules
Business rules In the repository or service In the blueprint
Guard coverage ruff and ast-grep rules ruff-ownership.toml:51 exempts src/admin/**
Behavioral coverage 2,628 scenarios, 5 transports 13 scenarios, 1 transport, 1 of 27 blueprints

The buyer plane reaches one seam by construction, which
#2232 §3 calls derivation and §5.3 calls
construction. serve() takes headers and never an identity, _resolve_identity runs once, and
the implementation receives (req, identity) and nothing else. An .ast-grep rule refuses any
other signature, so omitting the check is a type error rather than a silence.

The 33 count belongs to #2203, which measured
the live app.url_map. That method is authoritative here. A route that inherits <tenant_id>
from a blueprint url_prefix is invisible to any scan of decorator source, so only the URL map
sees it.

The ten mechanisms

Each row is a distinct way the admin plane answers "may this caller do this".

# Mechanism Location Owner
1 @require_auth(admin_only=) src/admin/utils/helpers.py:265, 25 applications
2 @require_tenant_access(api_mode=) helpers.py:304, 148 applications, ending in a select(User) at :363
3 require_api_key_auth() factory src/admin/auth_helpers.py:94, instantiated twice
4 A private third require_auth and get_tenant_access src/adapters/gam_reporting_api.py:60, :89, guarding six routes #2204
5 is_super_admin() reading a session cache it writes itself helpers.py:151, :164, :217:220
6 Inline session.get("role") in route bodies 20 sites #2203 Notes
7 Inline "user" not in session in undecorated routes 7 sites
8 Template gates on session.role templates/base.html:155, :165:179 #2206
9 A test-session bypass inside both decorators helpers.py:272, :320 #2235
10 Nothing at all 33 tenant-scoped URL rules #2203 AC 6

Three of these read as coverage and grant nothing.

User.role carries CheckConstraint("role IN ('admin', 'manager', 'viewer')")
(src/core/database/models.py:742). Mechanism 8 gates on super_admin and tenant_admin, which
that constraint forbids. #2203's Notes
establish that no production login path assigns either value, so mechanisms 6 and 8 are dead
branches. #2206 is the same fault one level
down: helpers.py:249 filters on is_admin=True, a column User does not declare, and the
resulting InvalidRequestError becomes an unconditional False.

Mechanism 5 sustains itself. is_super_admin() reads session["is_super_admin"] against
session["admin_email"], and _cache_admin_status writes both.

Why the seat drifted

ruff-ownership.toml:51 reads:

"src/admin/**" = ["TID251"]

The intent above it is legitimate: the admin UI handles principals and accounts as data rather
than acting as a caller. The glob is not. It exempts every file under src/admin/ from every
TID251 ban in that config, including bans added later. The same comment records that the four
single files outside src/admin and scripts are "listed by name so nothing else in src/
inherits the exemption". The admin app is the one surface that lost that discipline.

#2232 §11 rejects the alternative this
exemption produced: "Fix the issues individually. This is the current approach. Each fix is
correct, and none of them prevents the next instance." Five independently reported admin
authorization defects sit at a surface the guards do not read.

What grades the admin plane

The answer determines whether consolidation is safe to attempt, so it is measured rather than
assumed.

Measure Admin plane Buyer plane
Behavior-driven development (BDD) features 1 52
Scenarios 13 2,628
Transports 1 5
Blueprints covered 1 of 27
Scenarios asserting an authorization refusal 1

A further 190 tests outside BDD cover src/admin/, and 7 of them assert an authorization
refusal.

The suite also cannot detect a missing decorator. tests/admin/conftest.py:26 sets
ADCP_AUTH_TEST_MODE=true, which arms the bypass at helpers.py:272 and :320, so a decorated
handler and an undecorated one behave identically under test.
#2203 names the same hazard from the
test-authoring side: "Do not enable ADCP_AUTH_TEST_MODE or forge a super-admin session in a test
meant to prove authorization works."

Findings and their owners

Verified on feature/spec-gaps-1210, which sits one to two lines ahead of main. That offset
explains every line-number difference from
#2203#2206.

Finding Owner
Five publisher_partners.py routes unauthenticated PR #2075, in review
Four tenant-scoped routes with require_auth() and no tenant check #2203, PR #2231
The 33 unguarded rules, and the url_map guard #2203 AC 6, specified and unlanded
Dead inline session.get("role") branches #2203 Notes asks for an issue; unfiled
gam_reporting_api third require_auth, OIDC 500s, super-admins unscoped #2204
GAM OAuth callback takes the tenant from attacker-supplied state #2205
is_tenant_admin() queries a phantom User.is_admin #2206, blocks #1861
OIDC token_endpoint and jwks_uri dialled unvalidated #1872
Admin harness has no transport axis #2235, assigned
src/admin reaching past the repository and unit of work #1853
Admin handlers auditing refusals as successes #2166
Approval query with no tenant filter #2126, #2127
Per-account scope model and authoring surface #1615, #1856
Unsigned id_token accepted, email_verified unchecked Unowned. Equals #2232 §6 item 1
/auth/oidc/test/<tenant_id> undecorated, its callback sets oidc_enabled Unowned
No cross-site request forgery (CSRF) protection, while appearing to have it Unowned
A minted buyer token flashed into the session cookie Unowned
Tenant-management API key mint and verify disagree Unowned
An admin-created account no buyer can use Unowned
Admin account path bypasses every sync_accounts gate Unowned
No credential carries an expiry Unowned

The unowned findings that carry design content

The unsigned identity. src/admin/auth_utils.py:31:41 decodes the id_token with
verify_signature: False whenever token["userinfo"] is absent, and trusts the claims. The
string email_verified appears nowhere under src/. The email falls back to
preferred_username, then upn, then sub (:47:52). That value becomes the authorization
subject, and helpers.py:357 passes it to is_super_admin(), which grants on a domain match.
Per-tenant OIDC scopes are writable by any active User row of that tenant, and removing
openid from them moves that tenant onto the unsigned path.

This is #2232 §6's first identity-integrity
item, at mechanism level. §6 sequences that work before admin-plane enforcement, and this finding
is the evidence for that ordering rather than a new requirement. It wants an issue number.

Two surfaces disagree about one entity. sync_accounts applies approval mode, billing
policy, a sandbox capability check, and a field policy. The admin path applies none of them: it
hardcodes status="active", offers the full billing enum, writes sandbox unchecked, and reaches
build_row directly. AccountRepository.grant_access has one caller in the tree, inside the
tool. So an admin-created account never gets a grant, and _require_access refuses every later
buyer reference to it. The tool's update branch does not grant either, so a buyer syncing that
natural key receives action="updated" and an account_id that fails on every subsequent call.

These are business rules about an account, so they belong below both callers. That is the same
argument #2232 §5.3 makes for its repository
seam.

Sequencing

Each step lands independently and leaves the tree green.

  1. Build the net. #2235 is this step, and it
    says so: "Until this lands, new admin features should not be converted onto the current
    harness." Consolidating ten mechanisms without it changes authorization behavior with 13
    scenarios watching.
  2. Land the identity-integrity work that
    #2232 §6 sequences first, including the
    unsigned id_token. Enforcing policy against a forgeable subject converts a
    misconfiguration into a grant.
  3. Land #2203 AC 6, the url_map guard, so
    route 180 fails continuous integration on the day someone writes it.
  4. Narrow ruff-ownership.toml:51 from a glob to named files, matching the discipline its own
    comment describes.
  5. Retire mechanisms 4 through 9 onto mechanism 2, and have it return a Decision in
    #2232 §5.2's vocabulary.
  6. Move the account gates below both callers.

Open questions

  1. Does the admin plane's decision reach Decision through require_tenant_access, or through a
    before_request hook that covers unguarded rules by default? The second closes mechanism 10
    by construction and changes every route's failure mode at once.
  2. May an identity-provider-asserted email confer super-admin status at all? Today a domain match
    grants it, and per-tenant OIDC configuration is writable by any active User row of that
    tenant.
  3. What replaces the test-credential login path? ADCP_AUTH_TEST_MODE decides whether that
    blueprint is composed, which makes the app under test differ from the app deployed. It is
    also the only path to a first admin session without single sign-on, and the deployment
    documentation instructs operators to use it.

Sources

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions