internal/api: unify human/service-account authz into one Caller type #24

Merged
niklas merged 1 commits from unify-caller-authz into main 2026-10-02 20:06:38 +00:00
8 changed files with 502 additions and 80 deletions
+108 -31
View File
@@ -119,38 +119,100 @@ new key, revoke the old one," not "recreate the account."
### Auth middleware ### Auth middleware
`internal/api/middleware.go`'s existing dual resolution (`Authorization: Bearer` **Revised** (this section originally described an aspiration that didn't
→ `apiKeyUser()`, or session cookie → `sessionUser()`, both landing on the same match what shipped — `TEAM-LOOKUP.md` already caught one instance of that,
`models.User` + team-membership context) gains a third path: a bearer token that and a fuller audit found three more; this is the corrected, as-built
hashes to a `service_account_keys.key_hash` resolves to a distinct principal description, not the original proposal).
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.
**Team scope, as implemented, is owner-equivalent for every `requireTeamOwner` `internal/api/middleware.go`'s dual resolution (`Authorization: Bearer` →
endpoint, membership and invites included — nothing server-side carves those `apiKeyUser()`, or session cookie → `sessionUser()`) and the service-account
two out.** That's broader than what `terdut-operator`'s CRDs actually need path (`serviceAccountFor()`) both resolve into one `Caller` type
(escalation/deadman/integrations/OIDC-bindings only; membership is explicitly (`internal/api/caller.go`), not two parallel, un-unified context
never gitops-managed, see its DESIGN.md §4.2), a gap acknowledged rather than representations the way an earlier version of this server kept them. Every
closed here: narrowing this to exclude authorization predicate reads `Caller`'s methods:
`POST/DELETE /api/teams/{teamID}/members*` and
`.../invites*` specifically for a service-account caller is a small, isolated - `Caller.IsAdmin()` — true **only** for a human system administrator, never
follow-up (special-case those handlers rather than `requireTeamOwner` itself, for a service account of either scope, under any circumstance. `AdminOnly`
which every other owner-gated endpoint still wants shared). Until then, what and `requireSelfOrAdmin` key on this alone — user management
actually keeps membership out of automation's hands is that no operator built (`POST /api/users`, `PUT /api/users/{id}/admin`, etc.) and
against this scope should ever call those two endpoints — not a server-side `GET/PUT /api/admin/settings` stay human-only, forever. The original text
refusal. 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 ## 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 holding many credentials in one place (the operator's namespace) an
acceptable trade rather than reintroducing the mirrored design's acceptable trade rather than reintroducing the mirrored design's
server-admin-equivalent-everywhere problem. 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 ## Suggested sequencing
+5 -1
View File
@@ -279,7 +279,11 @@ type meResponse struct {
// between the login form and the app, since it cannot read its own cookie. // between the login form and the app, since it cannot read its own cookie.
func handleMe(db *sql.DB) http.HandlerFunc { func handleMe(db *sql.DB) http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) { return func(w http.ResponseWriter, r *http.Request) {
caller, _ := userFromContext(r.Context()) caller, ok := userFromContext(r.Context())
if !ok {
respond(w, http.StatusForbidden, errResp("this endpoint is for human accounts only"))
return
}
user, err := fetchUser(r.Context(), db, caller.ID) user, err := fetchUser(r.Context(), db, caller.ID)
if err != nil { if err != nil {
respond(w, http.StatusInternalServerError, errResp("internal error")) respond(w, http.StatusInternalServerError, errResp("internal error"))
+123
View File
@@ -0,0 +1,123 @@
package api
import (
"context"
"fmt"
"git.ryuvia.com/niklas/terdut-server/internal/models"
)
// Caller is the one principal type every authorization predicate in this
// package reads from. Before this, a human (ctxUser + ctxTeams) and a
// service account (ctxServiceAccount + a synthetic ctxTeams entry) were two
// parallel, un-unified context representations — every predicate had to
// remember which one(s) it needed to check, and the ones that forgot either
// 403'd a service account that should have been let through (terdut-server#23,
// terdut-operator#3), crashed on an unchecked zero-value user ID (handleMe,
// handleTestNotification), or silently no-op'd (handleDismissOnboarding).
// serveAs and serveAsServiceAccount now both build exactly one Caller and
// store it under one context key; everything else in this file is a read
// of one of its methods.
type Caller struct {
// user is set for a human caller (session cookie or a user's own API
// key), nil for a service account of either scope.
user *models.User
// sa is set for a service-account caller, nil for a human.
sa *serviceAccountPrincipal
// memberships is the caller's real team_members rows for a human, or —
// for a team-scoped service account — the single synthetic owner
// membership serveAsServiceAccount injects (see its own comment for
// why). Always nil for an instance-scoped service account: it acts on
// teams by id, not by belonging to one.
memberships []membership
}
// AsHuman returns the real user behind this caller, or false for a service
// account of either scope. Every handler that needs a real user_id to act
// on behalf of — not just "is this caller sufficiently privileged" — calls
// this and handles the false case explicitly, replacing the unchecked
// userFromContext(ctx) zero-value reads that used to silently misbehave for
// a service-account caller.
func (c Caller) AsHuman() (models.User, bool) {
if c.user == nil {
return models.User{}, false
}
return *c.user, true
}
// IsAdmin is true only for a human system administrator — never for a
// service account, of either scope, under any circumstance. AdminOnly and
// requireSelfOrAdmin key on this and nothing else: user management and
// /api/admin/settings stay human-only forever, by design (SERVICE-ACCOUNTS.md).
func (c Caller) IsAdmin() bool {
return c.user != nil && c.user.IsAdmin
}
// IsInstanceServiceAccount reports whether this caller is specifically an
// instance-scoped service account — never true for a human, including a
// human admin. handleCreateTeam needs exactly this: a human creates a team
// by being a human (and becomes its owner as a side effect), an
// instance-scoped service account creates one with no human owner at all;
// the two paths are not interchangeable, so this predicate must not also
// admit a human admin the way MayActAsInstanceAdmin deliberately does.
func (c Caller) IsInstanceServiceAccount() bool {
return c.sa != nil && c.sa.scope == models.ServiceAccountScopeInstance
}
// Role reports the caller's role in teamID, and whether they belong to it
// at all.
func (c Caller) Role(teamID int64) (string, bool) {
for _, m := range c.memberships {
if m.teamID == teamID {
return m.role, true
}
}
return "", false
}
// TeamIDs lists every team this caller belongs to: a human's real
// memberships, or a team-scoped service account's own single team. Always
// empty for an instance-scoped service account.
func (c Caller) TeamIDs() []int64 {
ids := make([]int64, 0, len(c.memberships))
for _, m := range c.memberships {
ids = append(ids, m.teamID)
}
return ids
}
// ServiceAccountID reports this caller's own service-account id, for the
// "may manage/rotate its own credential" self-check in
// callerMayManageServiceAccount, and for OperatorModeBlock's "any service
// account passes" rule.
func (c Caller) ServiceAccountID() (int64, bool) {
if c.sa == nil {
return 0, false
}
return c.sa.id, true
}
// Identity is a stable, log/audit-facing string distinguishing a human
// caller from a service account — "user:42" or "service-account:7". Not
// wired into any database column today (incidents.go's acknowledged_by/
// assigned_to/user_id are explicitly out of scope for this change — that
// needs its own schema migration, tracked separately), but this is the one
// place in the request path that already knows which kind of caller this
// is, and that follow-up will want exactly this accessor.
func (c Caller) Identity() string {
switch {
case c.user != nil:
return fmt.Sprintf("user:%d", c.user.ID)
case c.sa != nil:
return fmt.Sprintf("service-account:%d", c.sa.id)
default:
return "unknown"
}
}
func callerFromContext(ctx context.Context) (Caller, bool) {
c, ok := ctx.Value(ctxCaller).(Caller)
return c, ok
}
+29 -36
View File
@@ -16,10 +16,13 @@ import (
type contextKey string type contextKey string
const ( const (
ctxUser contextKey = "user" // ctxCaller holds the one Caller (see caller.go) every authorization
ctxSession contextKey = "session" // predicate in this package reads from — a human and a service account
ctxTeams contextKey = "teams" // used to be two parallel, un-unified context keys (ctxUser/ctxTeams vs.
ctxServiceAccount contextKey = "service_account" // ctxServiceAccount); this is why that was a mistake, not a smaller
// version of the same idea.
ctxCaller contextKey = "caller"
ctxSession contextKey = "session"
) )
// AuthMiddleware accepts either of the two credentials the server issues: an // AuthMiddleware accepts either of the two credentials the server issues: an
@@ -179,8 +182,7 @@ func serveAs(w http.ResponseWriter, r *http.Request, next http.Handler, db *sql.
return return
} }
ctx := context.WithValue(r.Context(), ctxTeams, teams) ctx := context.WithValue(r.Context(), ctxCaller, Caller{user: &u, memberships: teams})
ctx = context.WithValue(ctx, ctxUser, u)
if sessionID != 0 { if sessionID != 0 {
ctx = context.WithValue(ctx, ctxSession, sessionID) ctx = context.WithValue(ctx, ctxSession, sessionID)
} }
@@ -192,9 +194,13 @@ func hashToken(token string) string {
return hex.EncodeToString(h[:]) return hex.EncodeToString(h[:])
} }
// userFromContext is a thin compatibility wrapper over Caller.AsHuman(), so
// every call site written before the Caller abstraction (alerts.go,
// incidents.go, schedule.go, stats.go, and more) needs no change and keeps
// its exact existing behavior.
func userFromContext(ctx context.Context) (models.User, bool) { func userFromContext(ctx context.Context) (models.User, bool) {
u, ok := ctx.Value(ctxUser).(models.User) c, _ := callerFromContext(ctx)
return u, ok return c.AsHuman()
} }
// serviceAccountPrincipal is a service account as resolved from its key: // serviceAccountPrincipal is a service account as resolved from its key:
@@ -244,26 +250,21 @@ func serviceAccountFor(ctx context.Context, db *sql.DB, token string) (serviceAc
// key is only ever set by the client that holds it, never attached by a // key is only ever set by the client that holds it, never attached by a
// browser to a request another site makes. // browser to a request another site makes.
func serveAsServiceAccount(w http.ResponseWriter, r *http.Request, next http.Handler, sa serviceAccountPrincipal) { func serveAsServiceAccount(w http.ResponseWriter, r *http.Request, next http.Handler, sa serviceAccountPrincipal) {
ctx := r.Context() var memberships []membership
if sa.scope == models.ServiceAccountScopeTeam { if sa.scope == models.ServiceAccountScopeTeam {
ctx = context.WithValue(ctx, ctxTeams, []membership{{teamID: sa.teamID, role: models.RoleOwner}}) memberships = []membership{{teamID: sa.teamID, role: models.RoleOwner}}
} }
ctx = context.WithValue(ctx, ctxServiceAccount, sa) ctx := context.WithValue(r.Context(), ctxCaller, Caller{sa: &sa, memberships: memberships})
next.ServeHTTP(w, r.WithContext(ctx)) next.ServeHTTP(w, r.WithContext(ctx))
} }
func serviceAccountFromContext(ctx context.Context) (serviceAccountPrincipal, bool) { // isInstanceServiceAccount is a thin compatibility wrapper over
sa, ok := ctx.Value(ctxServiceAccount).(serviceAccountPrincipal) // Caller.IsInstanceServiceAccount(), for call sites outside this package's
return sa, ok // core predicates (handleCreateTeam, handleCreateServiceAccount) that
} // needed this exact, narrow check before the Caller abstraction existed.
// isInstanceServiceAccount reports whether the caller is an instance-scoped
// service account — the one identity allowed to create a team and mint a
// team-scoped account against any of them, the two things system
// administration can already do that this extends to automation.
func isInstanceServiceAccount(ctx context.Context) bool { func isInstanceServiceAccount(ctx context.Context) bool {
sa, ok := serviceAccountFromContext(ctx) c, _ := callerFromContext(ctx)
return ok && sa.scope == models.ServiceAccountScopeInstance return c.IsInstanceServiceAccount()
} }
// operatorReason marks a write that operator mode refused as such, distinct // operatorReason marks a write that operator mode refused as such, distinct
@@ -291,7 +292,8 @@ func OperatorModeBlock(cfg config.Config) func(http.Handler) http.Handler {
return next return next
} }
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if _, ok := serviceAccountFromContext(r.Context()); ok { caller, _ := callerFromContext(r.Context())
if _, ok := caller.ServiceAccountID(); ok {
next.ServeHTTP(w, r) next.ServeHTTP(w, r)
return return
} }
@@ -333,24 +335,15 @@ func callerMemberships(ctx context.Context, db *sql.DB, userID int64) ([]members
// administration is about accounts, not about reading other people's incidents, // administration is about accounts, not about reading other people's incidents,
// and an admin who needs to see a team's queue can add themselves to it. // and an admin who needs to see a team's queue can add themselves to it.
func callerTeamIDs(ctx context.Context) []int64 { func callerTeamIDs(ctx context.Context) []int64 {
ms, _ := ctx.Value(ctxTeams).([]membership) c, _ := callerFromContext(ctx)
ids := make([]int64, 0, len(ms)) return c.TeamIDs()
for _, m := range ms {
ids = append(ids, m.teamID)
}
return ids
} }
// callerRole reports the caller's role in one team, and whether they are in it // callerRole reports the caller's role in one team, and whether they are in it
// at all. // at all.
func callerRole(ctx context.Context, teamID int64) (string, bool) { func callerRole(ctx context.Context, teamID int64) (string, bool) {
ms, _ := ctx.Value(ctxTeams).([]membership) c, _ := callerFromContext(ctx)
for _, m := range ms { return c.Role(teamID)
if m.teamID == teamID {
return m.role, true
}
}
return "", false
} }
// requireTeamMember answers the request and reports false unless the caller // requireTeamMember answers the request and reports false unless the caller
+31 -8
View File
@@ -41,10 +41,14 @@ func callerIsAdmin(ctx context.Context) bool {
return ok && u.IsAdmin return ok && u.IsAdmin
} }
// callerOwnsTeam reports whether the caller is a human owner of teamID. Built // callerOwnsTeam reports whether the caller is owner-equivalent for teamID:
// on callerRole/ctxTeams like requireTeamOwner, but without writing a // a human owner, or that team's own team-scoped service account (its single
// response: callers here need to combine it with other ways of being // synthetic membership, serveAsServiceAccount — ratified in
// allowed, not stop at the first no. // SERVICE-ACCOUNTS.md as intentional, not an accident: a team-scoped
// credential is that team's owner's reach, full stop, membership and
// invites included). Built on callerRole like requireTeamOwner, but without
// writing a response: callers here need to combine it with other ways of
// being allowed, not stop at the first no.
func callerOwnsTeam(ctx context.Context, teamID int64) bool { func callerOwnsTeam(ctx context.Context, teamID int64) bool {
role, ok := callerRole(ctx, teamID) role, ok := callerRole(ctx, teamID)
return ok && role == models.RoleOwner return ok && role == models.RoleOwner
@@ -173,9 +177,24 @@ func fetchServiceAccount(ctx context.Context, db *sql.DB, id int64) (models.Serv
// callerMayManageServiceAccount reports whether the caller may mint or revoke // callerMayManageServiceAccount reports whether the caller may mint or revoke
// a key on sa: a system administrator, that team-scoped account's own human // a key on sa: a system administrator, that team-scoped account's own human
// owner, or the account rotating its own credential — which is not a // owner, the account rotating its own credential (not a privilege
// privilege escalation, the same reasoning requireSelfOrAdmin already rests // escalation, the same reasoning requireSelfOrAdmin already rests on for a
// on for a user's own API keys. // user's own API keys) — or, new, an instance-scoped service account
// managing any team-scoped account.
//
// That last branch closes terdut-operator#3: handleCreateServiceAccount
// already lets an instance-scoped caller *create* a team-scoped account for
// any team (the branch below it, isInstanceServiceAccount(ctx)) — this
// account didn't have an equivalent reach to *adopt or rotate* one it
// didn't just create in the same call, which is exactly the recovery path
// terdut-operator's own documented crash-window handling depends on
// (DESIGN.md §5's general adopt-on-conflict rule): a reconcile that creates
// the account successfully but crashes before persisting its credential
// locally retries into a 409, and without this branch the only available
// recovery — minting a fresh key on the now-existing account — 403'd
// forever, with no way out. Granting it here is not a new power: it
// mirrors the create-time reach this scope already has, just extended to
// the retry path DESIGN.md's own crash-window reasoning requires.
func callerMayManageServiceAccount(ctx context.Context, sa models.ServiceAccount) bool { func callerMayManageServiceAccount(ctx context.Context, sa models.ServiceAccount) bool {
if callerIsAdmin(ctx) { if callerIsAdmin(ctx) {
return true return true
@@ -183,7 +202,11 @@ func callerMayManageServiceAccount(ctx context.Context, sa models.ServiceAccount
if sa.TeamID != nil && callerOwnsTeam(ctx, *sa.TeamID) { if sa.TeamID != nil && callerOwnsTeam(ctx, *sa.TeamID) {
return true return true
} }
if self, ok := serviceAccountFromContext(ctx); ok && self.id == sa.ID { caller, _ := callerFromContext(ctx)
if id, ok := caller.ServiceAccountID(); ok && id == sa.ID {
return true
}
if sa.TeamID != nil && caller.IsInstanceServiceAccount() {
return true return true
} }
return false return false
+126
View File
@@ -181,10 +181,136 @@ func TestServiceAccount_TeamScopeManagesItsResources(t *testing.T) {
resp2.Body.Close() resp2.Body.Close()
} }
// Documents the capability already granted at create time (handleCreateServiceAccount's
// own callerOwnsTeam branch) also applies here: a team-scoped account is that
// team's owner's reach, membership and further accounts included, not just
// the handful of endpoints exercised above. Kept, not restricted, for
// symmetry with the now-ratified membership/invite capability below.
func TestServiceAccount_TeamScopeCanMintAnotherAccountForItsOwnTeam(t *testing.T) {
s := newTS(t)
instanceKey := createServiceAccount(t, s, s.key, "terdut-operator", models.ServiceAccountScopeInstance, 0)
teamA := createTeamAs(t, s, instanceKey, "team-a")
keyA := createServiceAccount(t, s, instanceKey, "team-a-sa", models.ServiceAccountScopeTeam, teamA)
resp := s.reqAs(t, keyA, http.MethodPost, "/api/service-accounts",
map[string]any{"name": "team-a-sa-2", "scope": models.ServiceAccountScopeTeam, "team_id": teamA})
if resp.StatusCode != http.StatusCreated {
t.Errorf("team-scoped account minting another account for its own team: %d", resp.StatusCode)
}
resp.Body.Close()
}
// SERVICE-ACCOUNTS.md ratifies this explicitly: a team-scoped account is
// owner-equivalent for every requireTeamOwner endpoint, membership and
// invites included — terdut-operator's own invite-minting feature depends on
// exactly this. No test exercised handleCreateInvite from a service account
// before this change, and it would have 500'd (created_by written as a bare
// zero value against a NOT-validated-but-FK'd column) rather than succeeded;
// see the signup_test.go addition for that half.
func TestServiceAccount_TeamScopeManagesItsOwnInvites(t *testing.T) {
s := newTS(t)
instanceKey := createServiceAccount(t, s, s.key, "terdut-operator", models.ServiceAccountScopeInstance, 0)
teamA := createTeamAs(t, s, instanceKey, "team-a")
keyA := createServiceAccount(t, s, instanceKey, "team-a-sa", models.ServiceAccountScopeTeam, teamA)
var invite struct {
ID int64 `json:"id"`
}
resp := s.reqAs(t, keyA, http.MethodPost, "/api/teams/"+id64(teamA)+"/invites", map[string]any{})
if resp.StatusCode != http.StatusCreated {
t.Fatalf("team-scoped account creating an invite: %d", resp.StatusCode)
}
decode(t, resp, &invite)
var list []map[string]any
decode(t, s.reqAs(t, keyA, http.MethodGet, "/api/teams/"+id64(teamA)+"/invites", nil), &list)
if len(list) != 1 {
t.Errorf("expected the invite to list back, got %d", len(list))
}
if resp := s.reqAs(t, keyA, http.MethodDelete,
"/api/teams/"+id64(teamA)+"/invites/"+id64(invite.ID), nil); resp.StatusCode != http.StatusNoContent {
t.Errorf("team-scoped account revoking its own invite: %d", resp.StatusCode)
} else {
resp.Body.Close()
}
}
// --------------------------------------------------------------------------- // ---------------------------------------------------------------------------
// Key rotation // Key rotation
// --------------------------------------------------------------------------- // ---------------------------------------------------------------------------
// terdut-operator#3: an instance-scoped account is already trusted to CREATE
// a team-scoped account for any team (handleCreateServiceAccount's own
// isInstanceServiceAccount branch) — this pins that it is equally trusted to
// manage/rotate a key on one that already exists and that it did not just
// create in this call, which is the exact shape of terdut-operator's own
// crash-window recovery (mint succeeds, a later step is interrupted before
// persisting the credential locally, and the next reconcile retries into a
// 409 then needs to mint a fresh key on the now-existing account). Before
// this fix, the second POST .../keys below 403'd forever.
func TestServiceAccount_InstanceScopeAdoptsAnExistingTeamScopedAccountsKey(t *testing.T) {
s := newTS(t)
instanceKey := createServiceAccount(t, s, s.key, "terdut-operator", models.ServiceAccountScopeInstance, 0)
teamA := createTeamAs(t, s, instanceKey, "team-a")
resp := s.reqAs(t, instanceKey, http.MethodPost, "/api/service-accounts",
map[string]any{"name": "team-a-sa", "scope": models.ServiceAccountScopeTeam, "team_id": teamA})
if resp.StatusCode != http.StatusCreated {
t.Fatalf("create team-scoped account: %d", resp.StatusCode)
}
var created struct {
ServiceAccount struct {
ID int64 `json:"id"`
} `json:"service_account"`
}
decode(t, resp, &created)
// Simulates the adopt-on-409 recovery path: this instance-scoped caller
// did not just create this account in this call (a fresh *tdclient.Client
// request, same as a second, independent reconcile would issue), yet
// still needs to mint it a fresh key.
rotateResp := s.reqAs(t, instanceKey, http.MethodPost,
"/api/service-accounts/"+id64(created.ServiceAccount.ID)+"/keys", map[string]string{"name": "adopted"})
if rotateResp.StatusCode != http.StatusCreated {
t.Errorf("instance-scoped account adopting a team-scoped account's key: %d", rotateResp.StatusCode)
}
rotateResp.Body.Close()
}
// ---------------------------------------------------------------------------
// AdminOnly / requireSelfOrAdmin — unchanged after the Caller refactor
// ---------------------------------------------------------------------------
// The Caller abstraction must not have widened AdminOnly/requireSelfOrAdmin:
// user management and /api/admin/settings stay human-only, for every scope
// of service account, exactly as before.
func TestAdminOnly_RefusesEveryServiceAccountScope(t *testing.T) {
s := newTS(t)
instanceKey := createServiceAccount(t, s, s.key, "terdut-operator", models.ServiceAccountScopeInstance, 0)
teamA := createTeamAs(t, s, instanceKey, "team-a")
teamKey := createServiceAccount(t, s, instanceKey, "team-a-sa", models.ServiceAccountScopeTeam, teamA)
for _, key := range []string{instanceKey, teamKey} {
if resp := s.reqAs(t, key, http.MethodGet, "/api/admin/settings", nil); resp.StatusCode != http.StatusForbidden {
t.Errorf("expected 403 for a service account reading /api/admin/settings, got %d", resp.StatusCode)
} else {
resp.Body.Close()
}
if resp := s.reqAs(t, key, http.MethodPost, "/api/users",
map[string]string{"username": "nope", "email": "nope@example.com"}); resp.StatusCode != http.StatusForbidden {
t.Errorf("expected 403 for a service account creating a user, got %d", resp.StatusCode)
} else {
resp.Body.Close()
}
if resp := s.reqAs(t, key, http.MethodGet, "/api/me", nil); resp.StatusCode != http.StatusForbidden {
t.Errorf("expected 403 for a service account calling /api/me, got %d", resp.StatusCode)
} else {
resp.Body.Close()
}
}
}
func TestServiceAccount_SelfRotatesItsOwnKey(t *testing.T) { func TestServiceAccount_SelfRotatesItsOwnKey(t *testing.T) {
s := newTS(t) s := newTS(t)
instanceKey := createServiceAccount(t, s, s.key, "terdut-operator", models.ServiceAccountScopeInstance, 0) instanceKey := createServiceAccount(t, s, s.key, "terdut-operator", models.ServiceAccountScopeInstance, 0)
+24 -4
View File
@@ -359,7 +359,19 @@ func handleCreateInvite(db *sql.DB, publicURL string) http.HandlerFunc {
respond(w, http.StatusInternalServerError, errResp("internal error")) respond(w, http.StatusInternalServerError, errResp("internal error"))
return return
} }
caller, _ := userFromContext(r.Context()) // created_by is nullable (ON DELETE SET NULL) for exactly this
// reason: the caller minting an invite is not always a human with a
// real users row. A team-scoped service account is owner-equivalent
// here (requireTeamOwner above already let it through), and this
// must leave created_by NULL for one the same way
// handleCreateServiceAccount already does for the analogous case —
// an unchecked zero value would violate the users(id) foreign key
// instead of recording "nobody" cleanly.
var createdBy *int64
if u, ok := userFromContext(r.Context()); ok {
id := u.ID
createdBy = &id
}
expires := time.Now().Add(inviteTTL) expires := time.Now().Add(inviteTTL)
var out inviteJSON var out inviteJSON
@@ -368,7 +380,7 @@ func handleCreateInvite(db *sql.DB, publicURL string) http.HandlerFunc {
INSERT INTO invites (token_hash, team_id, role, created_by, expires_at, max_uses) INSERT INTO invites (token_hash, team_id, role, created_by, expires_at, max_uses)
VALUES ($1, $2, $3, $4, $5, $6) VALUES ($1, $2, $3, $4, $5, $6)
RETURNING id, team_id, role, created_at, expires_at, max_uses, uses`, RETURNING id, team_id, role, created_at, expires_at, max_uses, uses`,
hash, teamID, req.Role, caller.ID, expires.Unix(), req.MaxUses). hash, teamID, req.Role, createdBy, expires.Unix(), req.MaxUses).
Scan(&out.ID, &out.TeamID, &out.Role, &created, &expiresAt, &out.MaxUses, &out.Uses); err != nil { Scan(&out.ID, &out.TeamID, &out.Role, &created, &expiresAt, &out.MaxUses, &out.Uses); err != nil {
respond(w, http.StatusInternalServerError, errResp("internal error")) respond(w, http.StatusInternalServerError, errResp("internal error"))
return return
@@ -429,7 +441,11 @@ func handleTestNotification(cfg NotifyConfig, db *sql.DB) http.HandlerFunc {
errResp("this server has no ntfy configured, so it can send nothing")) errResp("this server has no ntfy configured, so it can send nothing"))
return return
} }
caller, _ := userFromContext(r.Context()) caller, ok := userFromContext(r.Context())
if !ok {
respond(w, http.StatusForbidden, errResp("this endpoint is for human accounts only"))
return
}
var topic *string var topic *string
if err := db.QueryRowContext(r.Context(), if err := db.QueryRowContext(r.Context(),
@@ -470,7 +486,11 @@ func handleDismissOnboarding(db *sql.DB) http.HandlerFunc {
respond(w, http.StatusBadRequest, errResp("dismissed is required")) respond(w, http.StatusBadRequest, errResp("dismissed is required"))
return return
} }
caller, _ := userFromContext(r.Context()) caller, ok := userFromContext(r.Context())
if !ok {
respond(w, http.StatusForbidden, errResp("this endpoint is for human accounts only"))
return
}
var err error var err error
if *req.Dismissed { if *req.Dismissed {
+56
View File
@@ -2,10 +2,14 @@ package api_test
import ( import (
"bytes" "bytes"
"database/sql"
"encoding/json" "encoding/json"
"net/http" "net/http"
"net/http/cookiejar" "net/http/cookiejar"
"testing" "testing"
"git.ryuvia.com/niklas/terdut-server/internal/api"
"git.ryuvia.com/niklas/terdut-server/internal/models"
) )
// signup posts to the unauthenticated sign-up endpoint, the way the form does, // signup posts to the unauthenticated sign-up endpoint, the way the form does,
@@ -268,3 +272,55 @@ func TestSignup_ValidatesLikeTheRestOfTheServer(t *testing.T) {
t.Errorf("an existing username: expected 409, got %d", taken.StatusCode) t.Errorf("an existing username: expected 409, got %d", taken.StatusCode)
} }
} }
// A team-scoped service account has no users row to attribute created_by to.
// Before this fix, handleCreateInvite wrote its zero-value caller.ID straight
// into that (nullable, ON DELETE SET NULL) foreign key instead of leaving it
// NULL the way handleCreateServiceAccount already does for the same
// situation — a 500, not the 201 TestServiceAccount_TeamScopeManagesItsOwnInvites
// now confirms. This pins the column itself ends up NULL, not just "some
// response came back".
func TestSignup_InviteCreatedByAServiceAccountLeavesCreatedByNull(t *testing.T) {
s := newTS(t)
instanceKey := createServiceAccount(t, s, s.key, "terdut-operator", models.ServiceAccountScopeInstance, 0)
teamA := createTeamAs(t, s, instanceKey, "team-a")
keyA := createServiceAccount(t, s, instanceKey, "team-a-sa", models.ServiceAccountScopeTeam, teamA)
var created struct {
ID int64 `json:"id"`
}
decode(t, s.reqAs(t, keyA, http.MethodPost, "/api/teams/"+id64(teamA)+"/invites", map[string]any{}), &created)
var createdBy sql.NullInt64
if err := s.db.QueryRow("SELECT created_by FROM invites WHERE id = $1", created.ID).Scan(&createdBy); err != nil {
t.Fatalf("read back invites.created_by: %v", err)
}
if createdBy.Valid {
t.Errorf("expected created_by to be NULL for a service-account-minted invite, got %d", createdBy.Int64)
}
}
// Neither of these has a real user_id to act on behalf of; both must 403 a
// service account explicitly rather than 500 (handleTestNotification, which
// used to query ntfy_topic for user id 0) or silently no-op (handleDismissOnboarding,
// which used to UPDATE ... WHERE id = 0, affecting nothing and still
// returning 204).
func TestServiceAccount_HumanOnlyEndpointsRefuseExplicitly(t *testing.T) {
// BaseURL set (even to a fake, unreachable address) so handleTestNotification
// reaches its AsHuman() check instead of short-circuiting on "ntfy not
// configured" first — this test is about the human-only check, not ntfy.
s := newTS(t, api.NotifyConfig{BaseURL: "http://ntfy.invalid"})
instanceKey := createServiceAccount(t, s, s.key, "terdut-operator", models.ServiceAccountScopeInstance, 0)
if resp := s.reqAs(t, instanceKey, http.MethodPost, "/api/me/notify/test", nil); resp.StatusCode != http.StatusForbidden {
t.Errorf("expected 403 for a service account testing notifications, got %d", resp.StatusCode)
} else {
resp.Body.Close()
}
if resp := s.reqAs(t, instanceKey, http.MethodPut, "/api/me/onboarding",
map[string]bool{"dismissed": true}); resp.StatusCode != http.StatusForbidden {
t.Errorf("expected 403 for a service account dismissing onboarding, got %d", resp.StatusCode)
} else {
resp.Body.Close()
}
}