Enforce access checks on mutating routes + add coverage guard

[ruizhang0519]

Tracking: #2234 (step 0 of the RBAC enablement sequence).

RBAC is opt-in per endpoint: a mutation is only authorized if its handler reaches AccessChecker.check(). With the permissive default, an endpoint that never calls check() fails open silently — nothing turns red today, but it becomes a hole once a namespace is governed. Several mutating endpoints were uncovered (remove_complex_dimension_link built an access request and never checked it), and nothing guarded against the next one.

This PR makes the mutating-route surface covered and keeps it that way, with two guards that answer different questions:

• Is a check wired? tests/test_route_coverage.py enumerates every mutating route and asserts each reaches check() or is excluded with a reason. 67 routes reach a check; 30 are excluded across 7 reason buckets.
• Does the check bite? tests/api/write_enforcement_test.py asserts every mutating route has a denial test, is excluded, or is in a backlog with a reason. 19 cases execute (16 deny-WRITE, 3 deny-DELETE), covering 26 routes; 41 remain backlogged by reason.

The second guard exists because the first cannot tell a correct check from a wrong one. Review found exactly that case: upsert_materialization reached a READ check on a cube's metrics while writing to the node itself, so the coverage guard was green while transforms, dimensions and Druid cubes had no WRITE gate at all. A caller denied WRITE could create or overwrite a materialization config and reschedule its job. Fixed, with a denial test.

Checks added, each WRITE on the affected resource: dimension-link removal (node), namespace create (parent boundary), materialization writes including the upsert gap above (node), cube writes (cube node), and preaggregation writes (the node the pre-agg is based on).

Intentionally excluded: POST /preaggs/{id}/availability is a query-service callback, not a user action; governing it with user WRITE would break callbacks, so it awaits a service-identity model.

Also from review: create_or_reactivate_namespace moved to internal/namespaces.py next to the create_namespace it delegates to, and returns a NamespaceWriteResult instead of a JSONResponse so HTTP shaping stays in the endpoint. Status codes and bodies are unchanged. The already-exists case returns a status rather than raising, because register_table / register_view discard the result and only need the namespace to exist.

---

Why this won't take effect now: DJ's default access policy is permissive, so these checks allow requests unless an explicit restrictive grant or policy denies them.

When this will start taking effect: once a deployment enables restrictive RBAC and governs a namespace. Until then this is a no-op.

Out of scope (step-0 follow-ups): action/resource correctness, authorization on non-HTTP (deployment and background) paths, the availability service-identity model, and the 41 backlogged denial tests.

---

Verification:

1. Both guards, plus every executing denial case.

uv run pytest tests/api/write_enforcement_test.py tests/test_route_coverage.py
23 passed

2. Denial tests that need real fixtures, next to those fixtures.

uv run pytest tests/api/preaggregations_test.py
83 passed

3. The upsert_materialization gap, before and after.

# before the fix: the handler loaded the node and never authorized
POST /nodes/{node}/materialization -> 404
# after
POST /nodes/{node}/materialization -> 403 Access denied

4. Full server suite with the 100% coverage gate.

make test  ->  green, --cov-fail-under=100 satisfied

[netlify]

✅ Deploy Preview for thriving-cassata-78ae72 canceled.

Name	Link
🔨 Latest commit	a5b4519
🔍 Latest deploy log	https://app.netlify.com/projects/thriving-cassata-78ae72/deploys/6a68e508bb2b5e0007173d27

[shangyian]

Can this be moved into an internal/namespaces.py?

[ruizhang0519]

Moved it to internal/namespaces.py, next to create_namespace. Also dropped the JSONResponse return so HTTP shaping stays in the endpoint — it now returns a NamespaceWriteResult (created / reactivated / already_exists). Status codes and bodies unchanged.

Already-exists returns a status rather than raising, since register_* discard the result and just need the namespace to exist. Added a test to pin that.

[philipfweiss]

OK, so after some stress testing found an issue. upsert_materialization never checks WRITE on node_name.

create_new_materialization sends transforms and dimensions through build_non_cube_materialization_config, which is not passed an access_checker, and the DRUID_CUBE job through build_cube_materialization, which also does not check. The only branch that reaches .check() is the non-DRUID cube job, and that check is READ on the cube's metrics and dimensions, not WRITE on the node being materialized. So there is no WRITE gate on this endpoint for any node type.

Quick Example:
Let's say Restrictive RBAC governs finance; Alice has no grant on it.

POST /nodes/finance.orders/materializations/{name}/backfill returns 403

POST /nodes/finance.orders/materialization with a valid transform body returns 201: DJ writes the materialization, records history, and schedules the workflow. Because upsert updates in place, Alice can also overwrite the owner's existing config and reschedule it.

Suggested fix:
Add access_checker.add_request_by_node_name(node_name, ResourceAction.WRITE) and await access_checker.check(on_denied=AccessDenialMode.RAISE) at the top of the handler, matching deactivate and backfill.

[ruizhang0519]

Good catch — confirmed and fixed: added the WRITE check at the top of upsert_materialization, matching deactivate and backfill.

I also added a completeness test: every mutating route now needs a denial test or an explicit backlog entry. The denial suite is up to 21 cases (including a deny-DELETE variant), with 42 routes backlogged with reasons.

[philipfweiss]

OK- some small bugs/findings from an agent stress test:

1. Build the preaggregation measures once.
   The route currently calls build_measures_sql once for authorization and again during registration. The calls use different use_materialized values, so they can resolve different parent nodes. Please build once with dialect=Dialect.SPARK, use_materialized=False and pass that result into register_external_preaggregations.

2. Make namespace authorization consistent.
   The new endpoint checks WRITE on the parent namespace, while register_table, node creation, and restore check the target namespace. This conflicts with wildcard scopes such as finance.*. Please authorize the target namespace, or check both parent and target if the parent boundary is intentional. include_parents should also authorize every namespace it creates.

Examples:
Suppose a measure can use an available materialized parent. The authorization build uses use_materialized=True and may check permissions for that materialized parent. Registration rebuilds with use_materialized=False and may write using the base parent instead. The request is therefore authorized against one node set and executed against another.

Alice has WRITE on finance., which lets her write nodes under finance. When she creates finance.sub, the endpoint checks WRITE on finance. Since finance. does not match finance, she gets a 403. Giving her exact access to finance fixes namespace creation but no longer covers nodes below it.

Alice can still create the namespace through register_table, which checks finance.sub. That matches finance.*. The namespace is committed before catalog validation, so a request such as
POST /register/table/finance/sub/widgets/?source_node_namespace=
can create the namespace and then return 404.

Creating a.b.c.d with include_parents=true authorizes only a.b.c, even though it also creates a and a.b.

[philipfweiss]

The PR description is a bit confusing/roundabout- I think clearer is something like:

---

This PR adds missing WRITE authorization checks to namespace creation/reactivation, cube mutations, materialization configuration, preaggregation mutations, and dimension-link removal. It also fixes materialization upserts that previously checked READ on related metrics instead of WRITE on the node being changed.

It adds two regression guards: one verifies every mutating HTTP route reaches AccessChecker.check() or has a documented exclusion; the other verifies covered routes reject denied writes. These checks remain permissive until restrictive RBAC is enabled.

---

(Then explicit examples)

[philipfweiss]

LGTM pending comments being addressed

[philipfweiss]

One other quick thing- for these PRs, I find it not very helpful to have unit test coverage in pr description- that is tablestakes and also part of the CI checks.

Instead, I feel more effective is the structure of having an explicit runthrough of a case that fails pre-pr, and an explicit run through of the same case after the pr. That will help give the reviewer a better feel for what the change is doing and why.