Skip to content

Enforce access checks on mutating routes + add coverage guard#2334

Open
ruizhang0519 wants to merge 26 commits into
DataJunction:mainfrom
ruizhang0519:enforcement-coverage
Open

Enforce access checks on mutating routes + add coverage guard#2334
ruizhang0519 wants to merge 26 commits into
DataJunction:mainfrom
ruizhang0519:enforcement-coverage

Conversation

@ruizhang0519

@ruizhang0519 ruizhang0519 commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

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

RBAC enforcement is opt-in per endpoint: a mutation is only authorized if its handler reaches AccessChecker.check(). Because the default access policy is permissive, an endpoint that never calls check() fails open silently — nothing turns red today, and the gap only becomes exploitable once a namespace is governed. There was no test guaranteeing coverage, and several mutating endpoints were uncovered (including remove_complex_dimension_link, which built an access request but never called check()).

This PR makes the mutating-route surface completely and durably covered:

  • Route-coverage guard (tests/test_route_coverage.py): enumerates every mutating (POST/PUT/PATCH/DELETE) route from the app and asserts each either reaches check() (directly or via a helper) or is in an explicit allowlist with a reason. Two buckets: PENDING_COVERAGE (the step-0 backlog, which must reach empty) and INTENTIONAL_EXCLUSIONS (routes deliberately not node-governed). A companion test turns green once the backlog is empty, so "step-0 route coverage is complete" is a checkable signal.
  • Missing checks added, each governed by WRITE on the affected resource:
    • Dimension-link removal (remove_complex_dimension_link) — built requests but never called check().
    • Namespace create — WRITE on the parent boundary. Extracted create_or_reactivate_namespace so the HTTP endpoint (which enforces) and the internal register_table/register_view callers (which enforce their own) share the create logic without an endpoint-level check being skipped.
    • Materialization writes (deactivate, backfill) — WRITE on the node.
    • Cube writes (materialize, deactivate, backfill) — WRITE on the cube node.
    • Preaggregation writes (materialize, config, workflow, backfill, plan, register, bulk-deactivate) — WRITE on the node the pre-agg is based on (the metrics' parent nodes for plan/register).
  • Intentional exclusion: POST /preaggs/{id}/availability/ is a query-service completion callback, not a user action; governing it with user WRITE would break callbacks, so it is excluded pending a service-identity model.
  • Denial tests: verify each covered endpoint returns 403 when WRITE is denied. The guard proves check() is reached; these prove it enforces.

Why this won't take effect now: DJ's default access policy is still permissive, so the new checks allow requests when no explicit restrictive grant/policy denies them. This is a no-op today; it becomes relevant once a deployment enables restrictive RBAC and governs a namespace.


Out of scope (tracked as step-0 follow-ups):


Verification:

Automated (same as CI):

# Route-coverage guard: every mutating route reaches check() or is allowlisted,
# and PENDING_COVERAGE is empty (step-0 route coverage complete).
uv run pytest tests/test_route_coverage.py
4 passed

# Denial tests: each covered endpoint returns 403 when WRITE is denied.
uv run pytest tests/api/write_enforcement_test.py \
  tests/api/preaggregations_test.py::TestPreaggWriteEnforcement
10 passed

# Full server suite with the 100% coverage gate.
make test  ->  green, --cov-fail-under=100 satisfied

The denial tests install an authorization service that denies WRITE (the automated equivalent of a restrictive deployment) and assert 403 Access denied for: dimension-link removal, namespace create, materialization deactivate/backfill, cube materialize/deactivate/backfill, preaggregation bulk-deactivate, and preaggregation materialize/config/workflow/backfill/plan.

@netlify

netlify Bot commented Jul 16, 2026

Copy link
Copy Markdown

Deploy Preview for thriving-cassata-78ae72 canceled.

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

@ruizhang0519
ruizhang0519 force-pushed the enforcement-coverage branch from d2d4900 to ffc3e61 Compare July 17, 2026 01:39
@ruizhang0519
ruizhang0519 force-pushed the enforcement-coverage branch from ffc3e61 to 3c8787d Compare July 17, 2026 01:39
Comment thread datajunction-server/tests/test_route_coverage.py
@ruizhang0519 ruizhang0519 changed the title Add route-coverage guard for mutating endpoints [In Progress] Enforcement coverage + grant-authority lockdown Jul 17, 2026
@ruizhang0519 ruizhang0519 changed the title [In Progress] Enforcement coverage + grant-authority lockdown Enforcement coverage + grant-authority lockdown Jul 21, 2026
@ruizhang0519 ruizhang0519 changed the title Enforcement coverage + grant-authority lockdown Enforce access checks on mutating routes + add coverage guard Jul 21, 2026

@router.post("/namespaces/{namespace}/", status_code=HTTPStatus.CREATED)
async def create_node_namespace(
async def create_or_reactivate_namespace(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -24,8 +24,10 @@
from datajunction_server.internal.access.authentication.http import SecureAPIRouter

@philipfweiss philipfweiss Jul 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants