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() + } +}