Skip to content

refactor(server): make route registration and declaration the same act #7029

Description

@otavio

Problem Statement

A route in the API makes five claims about itself: the shape it answers with, the permission it
requires, whether an API key may use it, whether it deliberately reads across namespaces, and whether
it deliberately carries no actor.

Those claims are currently stated in three different places. The route shape, the unbounded claim and
the anonymous claim go to the gateway wrapper. The permission and the API-key policy stay behind as an
Echo middleware tail on the registration line. Anonymity is stated a second time, separately, in the
authenticator's allowlist.

Worse, a route declaration never learns its own address. The wrapper builds the declaration;
something else mounts the result. Nothing joins the two. The consequence is visible in the route-table
tests: they carry a hand-written list of the three routes converted so far, and recover a handler's
identity by reflecting on its function pointer and string-matching the name. Echo deliberately does not
expose a route's handler, so there is no way to close that join after the fact.

This costs us in three ways today:

  • A reviewer answering "can an API key rename a device?" reads three files.
  • The audits added alongside the shapes hold over a hand-maintained subset, not over the route table.
    They cannot catch a route that was mounted with no claim, a claim recorded for a route nobody mounted,
    or the same address mounted twice.
  • The conversion of the remaining ~160 handlers is about to rely on those audits. Right now they would
    be trusted rather than checked.

Solution

Make mounting a route and declaring it the same act.

Registration goes through the gateway, so a declaration is born knowing its method, its full path, its
permission and its API-key policy. One line states everything a route claims, and the route-table tests
stop being ledgers: they become invariants over every route the router mounts.

Nothing about request handling changes. The same guards run, in the same order, returning the same
responses. This is a change to how the route table is written and checked, not to what it does.

User Stories

  1. As a maintainer, I want a route's permission stated on the line that mounts it, so that I can read
    what a route requires without cross-referencing a middleware tail.
  2. As a reviewer, I want every claim a route makes to appear in one place, so that reviewing a new route
    is reading one line rather than reconciling three files.
  3. As a maintainer, I want the declaration to carry the address it was mounted at, so that a claim can
    be joined to its route without reflecting on function pointers.
  4. As a maintainer, I want a test that fails when a route is mounted with no declaration, so that a new
    route cannot silently escape the audit.
  5. As a maintainer, I want a test that fails when a declaration names a route nobody mounted, so that a
    stale claim is caught rather than believed.
  6. As a maintainer, I want a test that fails when the same method and path are mounted twice, so that a
    shadowed route is a build failure rather than a support ticket.
  7. As a maintainer, I want the anonymity claim checked against the authenticator's allowlist for every
    route rather than three, so that a route cannot be reachable without a credential by accident, or
    unreachable by typo.
  8. As a security reviewer, I want a route's declared permission to be provably the permission actually
    enforced, so that the declaration is evidence rather than documentation.
  9. As a contributor adding a route, I want the compiler and the tests to demand the claims, so that
    breadth, anonymity and authority cannot arrive by omission.
  10. As a maintainer, I want the declarations of one router to be readable independently of any other
    router built in the same process, so that an invariant over the route table does not depend on test
    order or on which edition a neighbouring test pinned.
  11. As an agent working in this codebase, I want a route's authority legible at its registration, so
    that I can reason about the route table without executing it.
  12. As a maintainer, I want the remaining conversion work to land against checked invariants, so that
    converting ~160 handlers is mechanical rather than an act of faith.

Implementation Decisions

The shapes return a route, not a handler. One, List, None and the legacy Handler adapter
stop returning a bare Echo handler and return a value pairing the handler with its pending declaration.
Route options move from the shape call to the mount call, so there is exactly one place per route where
its claims are written.

The gateway mounts. A small interface over Add(method, path, handler, middleware...) RouteInfo
satisfied by both the Echo instance and a group — lets one helper cover the API group, the admin groups
and the router root. The helper mounts, then completes the declaration from the returned route info.
This is the crux: the mounted address is only knowable at mount time, and it is the same string the
router reports and the authenticator matches on.

The declaration grows. It gains the method, the full path, the required permission, and whether the
route blocks API keys, alongside the shape and the existing unbounded and anonymous claims with their
required reasons.

gateway.GET(publicAPI, GetDeviceListURL,
    gateway.List(handler.GetDeviceList),
    gateway.Requires(authorizer.DeviceList))

The guards move into the gateway. The route middleware package imports the gateway, so the gateway
cannot import it back. The permission check and the API-key block move into the gateway — each needs
only the authorizer and the request headers, both already available there — and the existing middleware
names remain as thin wrappers so the cloud repo's direct callers keep compiling.

Only two guards become declarative. The permission check and the API-key block, which account for
the large majority of guard uses, are pure claims and move. The tenant guard and the legacy authorize
middleware stay as an explicit middleware tail. The latter in particular looks close to vestigial, and
deciding its fate is a behaviour question that must not ride inside a mechanical change.

Declarations belong to a router, not to the process. The current registry is a package-level set
filled as a side effect of wrapper construction, never reset, accumulating for the life of the binary.
That is benign only because declarations carry no address today; once they do, an edition-gated route
registered by one test leaves a claim that a later test will read as stale. The registry becomes a map
from router to that router's table, read back through an accessor taking the router. The process-global
accessor goes away. Tradeoff accepted: the map is keyed on router identity and never evicts, which is
bounded by routers created per process — one in production, a few dozen in a test binary.

The whole shellhub route table moves at once, including the routes still on the legacy adapter.
Those declare their address and permission even though their shape is still legacy. Moving only the
converted routes would keep the hand-maintained exempt ledger alive through the entire conversion,
which is the thing this change exists to remove.

Routes outside the gateway are named, with reasons. A set of routes are not gateway-wrapped at all
and never will be: the Prometheus endpoints, the MCP transport, the SSH connection and reverse-dial
endpoints, the web-terminal websockets, and the development-only pprof group. These are exempt from the
coverage invariant by name. One route deserves attention: the SSH package mounts a session-close
endpoint on the router root under the public API prefix, so it carries an /api address without
entering that group's chain. It is not a hole — the authenticator is installed at the root — but it is
the one route a group-based invariant would miss, and it is named rather than ignored.

Testing Decisions

Two seams, both already in use. Two rather than one because the router will not tell you which
middleware a route received: the claims are checkable statically, but that a claim is enforced is
only observable through behaviour.

Seam 1 — the declarations of a fully-built router. The existing route-table test seam, via the
package's authenticatedRouter helper, which builds the real router over a mocked service and pins the
Community edition. Extended from three routes to all of them:

  • every mounted route is covered by a declaration, or named in the exempt set with a reason
  • every declaration names a route that is actually mounted
  • no method and path pair is mounted twice
  • the anonymity claim and the authenticator's allowlist agree, over all routes
  • every unbounded or anonymous claim carries a non-blank reason

Prior art to follow: the existing check is a pure function over a slice of declarations, paired with a
mutation-style test proving the predicate bites rather than passing because it looks at nothing. Each
new invariant gets the same treatment — the predicate is a pure function, and a companion test feeds it
a known-bad input.

Seam 2 — HTTP through the built router. The seam every test in the routes package already uses.
This is what proves the change preserves behaviour: for a representative route per guard, a request
carrying a role without the permission is still refused, a request carrying an API key on a blocked
route is still refused, and a request missing both a namespace and a permission fails the same way it
did before. Guard ordering is the main risk in this change and this is where it is pinned.

Go, mockery via the generated service mock, no build tags, internal test package.

Out of Scope

  • Converting handler bodies. No handler changes shape here. The ~160 handlers still on the legacy
    adapter migrate per resource, in later work.
  • Adding, removing or changing any permission. A route that names none today still names none. The
    follow-up is where each such route gains either a permission or a written reason it needs none.
  • Collapsing the anonymous allowlist into the route table. Deferred: the allowlist must survive for
    routes that never touch the gateway, and registering from the mount call needs an authenticator the
    helper does not have and which is frequently absent. The all-routes anonymity invariant added here is
    what makes that follow-up safe to do later.
  • The tenant guard and the legacy authorize middleware.
  • The list-query contract, which moves into the list shape separately.
  • Cloud. The cloud repo adopts the same registration in its own stacked change; it currently
    registers no declarations at all, so the audits do not see it.
  • The session-close endpoint's registration inconsistency. Named, not fixed.

Further Notes

The rewrite aliases are pre-router middleware and create no routes, so they need no declarations and are
invisible to the router's route table. This is also why the health check is reachable anonymously
without its own allowlist entry — it rewrites onto a path that has one.

One finding to carry into the permission follow-up rather than this change: the SSH identity delete route
registers with no permission while its two siblings require one. It is guarded, in the handler body, so
the behaviour is correct — but the route table under-reports it, which is exactly the class of thing
declaring the permission at the registration is meant to end.

Note for CI: a pull request based on a feature branch runs no checks in this repository, because the
workflows are gated on the default branch. Lint and tests must be run locally, in-container, before this
is called green.

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

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions