internal/api: unify human/service-account authz into one Caller type
ctxUser/ctxTeams (human) and ctxServiceAccount (+ a synthetic ctxTeams
entry, service account) used to be two parallel, un-unified context
representations -- every authz predicate had to remember which one(s) it
needed, and every place that forgot either wrongly 403'd a service account
(terdut-server#23, terdut-operator#3), crashed on an unchecked zero-value
user id, or silently no-op'd. New internal/api/caller.go collapses both
into one Caller, stored under one ctxCaller key by serveAs/serveAsServiceAccount;
every existing predicate (userFromContext, callerTeamIDs, callerRole,
callerIsAdmin, isInstanceServiceAccount, AdminOnly, requireSelfOrAdmin,
requireTeamOwner, OperatorModeBlock) now reads through it, with identical
behavior for every untouched call site (alerts.go, incidents.go,
schedule.go, stats.go, etc.) -- confirmed by the full existing suite
passing unchanged.
Four real fixes land alongside the refactor, not just the restructuring:
1. callerMayManageServiceAccount gains the one load-bearing branch this
exists for: an instance-scoped service account may now manage (mint or
revoke a key on) any team-scoped account, not only a human admin, that
team's human owner, or the account itself. handleCreateServiceAccount
already let an instance-scoped caller *create* a team-scoped account for
any team; adopting or rotating one it didn't just create in the same
call -- terdut-operator's own documented crash-window recovery -- had no
equivalent permission and 403'd forever. Closes terdut-operator#3.
2. handleCreateInvite wrote a service-account caller's zero-value user id
straight into invites.created_by (nullable, but never passed as nil),
which foreign-key-violates against users(id) -- a 500, not success, for
any team-scoped service account minting an invite. Fixed the same way
handleCreateServiceAccount already handles the analogous case. Found
live while verifying this change, not filed separately since it's fixed
in the same place it was found.
3. handleMe and handleTestNotification 500'd for a service-account caller
(fetchUser/the ntfy_topic lookup against a zero-value user id that
matches no row); handleDismissOnboarding silently no-op'd (UPDATE ...
WHERE id = 0). All three now call Caller.AsHuman() and return an
explicit 403 ("this endpoint is for human accounts only").
4. Ratifies, rather than further narrows, two capabilities a team-scoped
service account already had by construction and this document's own
text once called "a gap acknowledged rather than closed": owner-equivalent
reach over membership/invites, and minting another service account for
its own team. terdut-operator's new TerdutTeam invite-minting feature is
about to depend on the first one, so this makes it documented, tested,
intentional behavior instead of an accident nobody was supposed to rely
on.
AdminOnly/requireSelfOrAdmin are unchanged in effect: still human-only,
forever, for every scope of service account -- confirmed by
TestAdminOnly_RefusesEveryServiceAccountScope. terdut-server#23's named
routes (POST /api/users, PUT /api/admin/settings) were never the right
thing to widen; its real fix is the terdut-operator invite feature,
recorded in SERVICE-ACCOUNTS.md's "What this unblocks" and closing that
issue once it ships.
SERVICE-ACCOUNTS.md amended in place (not a new file, its own established
convention) to describe the as-built Caller model, correct its own
aspirational claim about AdminOnly that TEAM-LOOKUP.md had already flagged
as not matching shipped code, and record all of the above.
This commit is contained in:
+108
-31
@@ -119,38 +119,100 @@ new key, revoke the old one," not "recreate the account."
|
||||
|
||||
### Auth middleware
|
||||
|
||||
`internal/api/middleware.go`'s existing dual resolution (`Authorization: Bearer`
|
||||
→ `apiKeyUser()`, or session cookie → `sessionUser()`, both landing on the same
|
||||
`models.User` + team-membership context) gains a third path: a bearer token that
|
||||
hashes to a `service_account_keys.key_hash` resolves to a distinct principal
|
||||
type, not a synthesized `models.User`. `requireTeamMember`/`requireTeamOwner`
|
||||
treat a matching team-scoped service account as owner-equivalent for that one
|
||||
team (satisfies the same checks a real team owner would), and an instance-scoped
|
||||
one as satisfying `AdminOnly` for team-creation/listing purposes **and** for
|
||||
minting a `team`-scoped service account against any team (`POST
|
||||
/api/service-accounts {"scope":"team","teamID":...}`) — this second permission
|
||||
is what lets an operator-style caller create a team, then immediately mint that
|
||||
team its own narrower credential, without a human in the loop for every team.
|
||||
Neither permission extends to user-management endpoints (`POST /api/users`,
|
||||
`PUT /api/users/{id}/admin`, etc.), which stay human-admin-only. Anywhere
|
||||
identity is recorded for a human (incident timeline
|
||||
`acknowledged_by`/`assigned_to`, audit-relevant fields), a service-account
|
||||
principal is stored and displayed distinctly, e.g. `service-account:terdut-operator`,
|
||||
never coerced into a `user_id` FK.
|
||||
**Revised** (this section originally described an aspiration that didn't
|
||||
match what shipped — `TEAM-LOOKUP.md` already caught one instance of that,
|
||||
and a fuller audit found three more; this is the corrected, as-built
|
||||
description, not the original proposal).
|
||||
|
||||
**Team scope, as implemented, is owner-equivalent for every `requireTeamOwner`
|
||||
endpoint, membership and invites included — nothing server-side carves those
|
||||
two out.** That's broader than what `terdut-operator`'s CRDs actually need
|
||||
(escalation/deadman/integrations/OIDC-bindings only; membership is explicitly
|
||||
never gitops-managed, see its DESIGN.md §4.2), a gap acknowledged rather than
|
||||
closed here: narrowing this to exclude
|
||||
`POST/DELETE /api/teams/{teamID}/members*` and
|
||||
`.../invites*` specifically for a service-account caller is a small, isolated
|
||||
follow-up (special-case those handlers rather than `requireTeamOwner` itself,
|
||||
which every other owner-gated endpoint still wants shared). Until then, what
|
||||
actually keeps membership out of automation's hands is that no operator built
|
||||
against this scope should ever call those two endpoints — not a server-side
|
||||
refusal.
|
||||
`internal/api/middleware.go`'s dual resolution (`Authorization: Bearer` →
|
||||
`apiKeyUser()`, or session cookie → `sessionUser()`) and the service-account
|
||||
path (`serviceAccountFor()`) both resolve into one `Caller` type
|
||||
(`internal/api/caller.go`), not two parallel, un-unified context
|
||||
representations the way an earlier version of this server kept them. Every
|
||||
authorization predicate reads `Caller`'s methods:
|
||||
|
||||
- `Caller.IsAdmin()` — true **only** for a human system administrator, never
|
||||
for a service account of either scope, under any circumstance. `AdminOnly`
|
||||
and `requireSelfOrAdmin` key on this alone — user management
|
||||
(`POST /api/users`, `PUT /api/users/{id}/admin`, etc.) and
|
||||
`GET/PUT /api/admin/settings` stay human-only, forever. The original text
|
||||
here claimed an instance-scoped service account satisfies `AdminOnly` "for
|
||||
team-creation/listing purposes" — that was never true of the shipped code
|
||||
(`TEAM-LOOKUP.md` caught the listing half; the creation half was always a
|
||||
separate, bespoke check in `handleCreateTeam`, not `AdminOnly` itself) and
|
||||
is not being made true now. Don't widen `AdminOnly`: every time this has
|
||||
come up, the fix has been a narrower, purpose-built capability instead
|
||||
(`?name=` lookups for teams and service accounts; now
|
||||
`terdut-operator`'s own invite-minting feature for the one real gap this
|
||||
boundary left — how a human ever gets a first login on a no-OIDC,
|
||||
operator-managed install. See the bottom of "What this unblocks.")
|
||||
- `Caller.IsInstanceServiceAccount()` — true only for an instance-scoped
|
||||
service account, never for a human (including a human admin).
|
||||
`handleCreateTeam` uses exactly this: a human creates a team by being a
|
||||
human (and becomes its owner); an instance-scoped service account creates
|
||||
one with no human owner at all. The two paths are not interchangeable, so
|
||||
this predicate deliberately does not also admit a human admin.
|
||||
- `Caller.Role(teamID)`/`TeamIDs()` — a human's real `team_members` rows, or
|
||||
a team-scoped service account's single synthetic owner membership
|
||||
(`serveAsServiceAccount`). This is what makes `requireTeamMember`/
|
||||
`requireTeamOwner` treat a team-scoped service account as owner-equivalent
|
||||
for that one team, with no separate branch needed in either function.
|
||||
- `Caller.ServiceAccountID()` — used by `OperatorModeBlock` ("any service
|
||||
account passes") and by `callerMayManageServiceAccount`'s self-rotation
|
||||
check.
|
||||
- `Caller.AsHuman()` — the accessor every handler that needs a real
|
||||
`user_id` to act on behalf of must call and check, instead of reading a
|
||||
user off context unconditionally. Before the `Caller` type existed, four
|
||||
handlers did the latter and silently misbehaved for a service-account
|
||||
caller: `handleMe` and `handleTestNotification` 500'd (a zero-value user id
|
||||
that matches no row), `handleDismissOnboarding` silently no-op'd (`UPDATE
|
||||
... WHERE id = 0` affects nothing, still returns 204), and
|
||||
`handleCreateInvite` wrote that same zero value into `invites.created_by`
|
||||
— a real foreign-key violation, not just a wrong answer, since that column
|
||||
is nullable but was never passed as `nil`. All four now call `AsHuman()`
|
||||
and return an explicit 403 ("this endpoint is for human accounts only")
|
||||
or, for the invite case, leave `created_by` `NULL` the same way
|
||||
`handleCreateServiceAccount` already did for the analogous situation.
|
||||
|
||||
**Team scope is owner-equivalent for every `requireTeamOwner` endpoint,
|
||||
membership and invites included — by design, not by an unclosed gap.** An
|
||||
earlier version of this document flagged this as "acknowledged rather than
|
||||
closed," kept in check only by the social convention that nobody *builds*
|
||||
automation against those two routes. That convention is retired:
|
||||
`terdut-operator`'s `TerdutTeam` controller now mints and revokes its own
|
||||
team's invite link through exactly this capability (its existing
|
||||
team-scoped credential, `POST`/`DELETE /api/teams/{teamID}/invites`), which
|
||||
is the real fix for the human-onboarding gap below — not a narrower
|
||||
carve-out of this capability. `service_accounts_test.go`'s
|
||||
`TestServiceAccount_TeamScopeManagesItsOwnInvites` pins it.
|
||||
|
||||
**A team-scoped account can also mint another service account scoped to its
|
||||
own team** (`handleCreateServiceAccount`'s `callerOwnsTeam` branch, which a
|
||||
team-scoped caller already satisfies for its own team via the synthetic
|
||||
membership above). Kept, not restricted, for the same reason: a team-scoped
|
||||
credential is that team's owner's reach, full stop — carving this one
|
||||
capability out while leaving membership/invites alone would be an arbitrary
|
||||
asymmetry. Pinned by
|
||||
`TestServiceAccount_TeamScopeCanMintAnotherAccountForItsOwnTeam`.
|
||||
|
||||
**`callerMayManageServiceAccount` gained the one load-bearing fix this
|
||||
redesign exists for:** an instance-scoped service account may manage
|
||||
(mint/revoke a key on) *any* team-scoped account, not only one admin, that
|
||||
team's human owner, or the account itself. `handleCreateServiceAccount`
|
||||
already let an instance-scoped caller *create* a team-scoped account for
|
||||
any team; this closes the gap where adopting or rotating one it didn't just
|
||||
create in the same call — exactly `terdut-operator`'s documented
|
||||
adopt-on-409 crash-window recovery (its own `DESIGN.md` §5) — 403'd forever
|
||||
instead of succeeding (`terdut-operator#3`). Pinned by
|
||||
`TestServiceAccount_InstanceScopeAdoptsAnExistingTeamScopedAccountsKey`.
|
||||
|
||||
Anywhere identity is recorded for a human (incident timeline
|
||||
`acknowledged_by`/`assigned_to`, audit-relevant fields), a service-account
|
||||
caller is still coerced into a bare `user_id` of `0` today — `Caller`'s new
|
||||
`Identity()` accessor exists for exactly this follow-up, but wiring it in
|
||||
needs a schema migration (an actor-attribution column distinct from
|
||||
`user_id`) and is deliberately out of scope here. Tracked separately, not by
|
||||
this document.
|
||||
|
||||
## What this unblocks
|
||||
|
||||
@@ -177,6 +239,21 @@ Directly resolves `terdut-operator` DESIGN.md §6's two broken assumptions:
|
||||
holding many credentials in one place (the operator's namespace) an
|
||||
acceptable trade rather than reintroducing the mirrored design's
|
||||
server-admin-equivalent-everywhere problem.
|
||||
4. **A human can get a first login on a no-OIDC, operator-managed install —
|
||||
without ever touching `AdminOnly` or `/api/admin/settings`.** This was
|
||||
filed as `terdut-server#23` ("no API path to create a human login after
|
||||
bootstrap") and diagnosed, at the time, as this server needing to let a
|
||||
service account through `AdminOnly`. It doesn't: the fix lives entirely
|
||||
in `terdut-operator`, because a team-scoped credential was *already*
|
||||
owner-equivalent for `POST /api/teams/{teamID}/invites`, and invite
|
||||
redemption (`POST /api/signup` with an `invite` token) bypasses
|
||||
`signup_mode` entirely — `terdut-operator` just never grew a feature to
|
||||
use either fact. Its `TerdutTeam` controller now mints and surfaces one
|
||||
via its own existing team-scoped credential (`spec.invite`,
|
||||
`status.inviteSecretRef`, see that repo's own docs), so a human joins a
|
||||
CRD-managed team by a real invite link, the same way anyone else would.
|
||||
`terdut-server#23` is closed with this note once that feature ships — its
|
||||
named routes stay human-only, correctly, not a gap.
|
||||
|
||||
## Suggested sequencing
|
||||
|
||||
|
||||
Reference in New Issue
Block a user