From 774fdfcaa89cf5e8f43276681de64a1f1a7bbcc1 Mon Sep 17 00:00:00 2001 From: Niklas Ye Date: Fri, 2 Oct 2026 21:52:43 +0200 Subject: [PATCH] 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. --- SERVICE-ACCOUNTS.md | 139 ++++++++++++++++++++------ internal/api/auth.go | 6 +- internal/api/caller.go | 123 +++++++++++++++++++++++ internal/api/middleware.go | 65 ++++++------ internal/api/service_accounts.go | 39 ++++++-- internal/api/service_accounts_test.go | 126 +++++++++++++++++++++++ internal/api/signup.go | 28 +++++- internal/api/signup_test.go | 56 +++++++++++ 8 files changed, 502 insertions(+), 80 deletions(-) create mode 100644 internal/api/caller.go diff --git a/SERVICE-ACCOUNTS.md b/SERVICE-ACCOUNTS.md index d34c2b7..2d264c0 100644 --- a/SERVICE-ACCOUNTS.md +++ b/SERVICE-ACCOUNTS.md @@ -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 diff --git a/internal/api/auth.go b/internal/api/auth.go index bb3b840..17f94fc 100644 --- a/internal/api/auth.go +++ b/internal/api/auth.go @@ -279,7 +279,11 @@ type meResponse struct { // between the login form and the app, since it cannot read its own cookie. func handleMe(db *sql.DB) http.HandlerFunc { 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) if err != nil { respond(w, http.StatusInternalServerError, errResp("internal error")) diff --git a/internal/api/caller.go b/internal/api/caller.go new file mode 100644 index 0000000..3370870 --- /dev/null +++ b/internal/api/caller.go @@ -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 +} diff --git a/internal/api/middleware.go b/internal/api/middleware.go index fc2204f..0aa314a 100644 --- a/internal/api/middleware.go +++ b/internal/api/middleware.go @@ -16,10 +16,13 @@ import ( type contextKey string const ( - ctxUser contextKey = "user" - ctxSession contextKey = "session" - ctxTeams contextKey = "teams" - ctxServiceAccount contextKey = "service_account" + // ctxCaller holds the one Caller (see caller.go) every authorization + // predicate in this package reads from — a human and a service account + // used to be two parallel, un-unified context keys (ctxUser/ctxTeams vs. + // 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 @@ -179,8 +182,7 @@ func serveAs(w http.ResponseWriter, r *http.Request, next http.Handler, db *sql. return } - ctx := context.WithValue(r.Context(), ctxTeams, teams) - ctx = context.WithValue(ctx, ctxUser, u) + ctx := context.WithValue(r.Context(), ctxCaller, Caller{user: &u, memberships: teams}) if sessionID != 0 { ctx = context.WithValue(ctx, ctxSession, sessionID) } @@ -192,9 +194,13 @@ func hashToken(token string) string { 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) { - u, ok := ctx.Value(ctxUser).(models.User) - return u, ok + c, _ := callerFromContext(ctx) + return c.AsHuman() } // 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 // browser to a request another site makes. func serveAsServiceAccount(w http.ResponseWriter, r *http.Request, next http.Handler, sa serviceAccountPrincipal) { - ctx := r.Context() + var memberships []membership 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)) } -func serviceAccountFromContext(ctx context.Context) (serviceAccountPrincipal, bool) { - sa, ok := ctx.Value(ctxServiceAccount).(serviceAccountPrincipal) - return sa, ok -} - -// 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. +// isInstanceServiceAccount is a thin compatibility wrapper over +// Caller.IsInstanceServiceAccount(), for call sites outside this package's +// core predicates (handleCreateTeam, handleCreateServiceAccount) that +// needed this exact, narrow check before the Caller abstraction existed. func isInstanceServiceAccount(ctx context.Context) bool { - sa, ok := serviceAccountFromContext(ctx) - return ok && sa.scope == models.ServiceAccountScopeInstance + c, _ := callerFromContext(ctx) + return c.IsInstanceServiceAccount() } // 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 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) 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, // and an admin who needs to see a team's queue can add themselves to it. func callerTeamIDs(ctx context.Context) []int64 { - ms, _ := ctx.Value(ctxTeams).([]membership) - ids := make([]int64, 0, len(ms)) - for _, m := range ms { - ids = append(ids, m.teamID) - } - return ids + c, _ := callerFromContext(ctx) + return c.TeamIDs() } // callerRole reports the caller's role in one team, and whether they are in it // at all. func callerRole(ctx context.Context, teamID int64) (string, bool) { - ms, _ := ctx.Value(ctxTeams).([]membership) - for _, m := range ms { - if m.teamID == teamID { - return m.role, true - } - } - return "", false + c, _ := callerFromContext(ctx) + return c.Role(teamID) } // requireTeamMember answers the request and reports false unless the caller diff --git a/internal/api/service_accounts.go b/internal/api/service_accounts.go index 90f7afd..c03bfcd 100644 --- a/internal/api/service_accounts.go +++ b/internal/api/service_accounts.go @@ -41,10 +41,14 @@ func callerIsAdmin(ctx context.Context) bool { return ok && u.IsAdmin } -// callerOwnsTeam reports whether the caller is a human owner of teamID. Built -// on callerRole/ctxTeams 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. +// callerOwnsTeam reports whether the caller is owner-equivalent for teamID: +// a human owner, or that team's own team-scoped service account (its single +// synthetic membership, serveAsServiceAccount — ratified in +// 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 { role, ok := callerRole(ctx, teamID) 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 // 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 -// privilege escalation, the same reasoning requireSelfOrAdmin already rests -// on for a user's own API keys. +// owner, the account rotating its own credential (not a privilege +// escalation, the same reasoning requireSelfOrAdmin already rests on for a +// 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 { if callerIsAdmin(ctx) { return true @@ -183,7 +202,11 @@ func callerMayManageServiceAccount(ctx context.Context, sa models.ServiceAccount if sa.TeamID != nil && callerOwnsTeam(ctx, *sa.TeamID) { 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 false diff --git a/internal/api/service_accounts_test.go b/internal/api/service_accounts_test.go index 559b25e..ffb7776 100644 --- a/internal/api/service_accounts_test.go +++ b/internal/api/service_accounts_test.go @@ -181,10 +181,136 @@ func TestServiceAccount_TeamScopeManagesItsResources(t *testing.T) { 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 // --------------------------------------------------------------------------- +// 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) { s := newTS(t) instanceKey := createServiceAccount(t, s, s.key, "terdut-operator", models.ServiceAccountScopeInstance, 0) diff --git a/internal/api/signup.go b/internal/api/signup.go index 64ad25e..7bdbd2c 100644 --- a/internal/api/signup.go +++ b/internal/api/signup.go @@ -359,7 +359,19 @@ func handleCreateInvite(db *sql.DB, publicURL string) http.HandlerFunc { respond(w, http.StatusInternalServerError, errResp("internal error")) 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) 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) VALUES ($1, $2, $3, $4, $5, $6) 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 { respond(w, http.StatusInternalServerError, errResp("internal error")) 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")) 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 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")) 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 if *req.Dismissed { diff --git a/internal/api/signup_test.go b/internal/api/signup_test.go index 79a5d90..7605df6 100644 --- a/internal/api/signup_test.go +++ b/internal/api/signup_test.go @@ -2,10 +2,14 @@ package api_test import ( "bytes" + "database/sql" "encoding/json" "net/http" "net/http/cookiejar" "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, @@ -268,3 +272,55 @@ func TestSignup_ValidatesLikeTheRestOfTheServer(t *testing.T) { 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() + } +} -- 2.52.0