Compare commits
5 Commits
| Author | SHA1 | Date | |
|---|---|---|---|
| 4358e84b24 | |||
| f45dc2f925 | |||
| 774fdfcaa8 | |||
| fd26fef1ba | |||
| a9d788cc83 |
+108
-31
@@ -119,38 +119,100 @@ new key, revoke the old one," not "recreate the account."
|
||||
|
||||
### Auth middleware
|
||||
|
||||
`internal/api/middleware.go`'s existing dual resolution (`Authorization: Bearer`
|
||||
→ `apiKeyUser()`, or session cookie → `sessionUser()`, both landing on the same
|
||||
`models.User` + team-membership context) gains a third path: a bearer token that
|
||||
hashes to a `service_account_keys.key_hash` resolves to a distinct principal
|
||||
type, not a synthesized `models.User`. `requireTeamMember`/`requireTeamOwner`
|
||||
treat a matching team-scoped service account as owner-equivalent for that one
|
||||
team (satisfies the same checks a real team owner would), and an instance-scoped
|
||||
one as satisfying `AdminOnly` for team-creation/listing purposes **and** for
|
||||
minting a `team`-scoped service account against any team (`POST
|
||||
/api/service-accounts {"scope":"team","teamID":...}`) — this second permission
|
||||
is what lets an operator-style caller create a team, then immediately mint that
|
||||
team its own narrower credential, without a human in the loop for every team.
|
||||
Neither permission extends to user-management endpoints (`POST /api/users`,
|
||||
`PUT /api/users/{id}/admin`, etc.), which stay human-admin-only. Anywhere
|
||||
identity is recorded for a human (incident timeline
|
||||
`acknowledged_by`/`assigned_to`, audit-relevant fields), a service-account
|
||||
principal is stored and displayed distinctly, e.g. `service-account:terdut-operator`,
|
||||
never coerced into a `user_id` FK.
|
||||
**Revised** (this section originally described an aspiration that didn't
|
||||
match what shipped — `TEAM-LOOKUP.md` already caught one instance of that,
|
||||
and a fuller audit found three more; this is the corrected, as-built
|
||||
description, not the original proposal).
|
||||
|
||||
**Team scope, as implemented, is owner-equivalent for every `requireTeamOwner`
|
||||
endpoint, membership and invites included — nothing server-side carves those
|
||||
two out.** That's broader than what `terdut-operator`'s CRDs actually need
|
||||
(escalation/deadman/integrations/OIDC-bindings only; membership is explicitly
|
||||
never gitops-managed, see its DESIGN.md §4.2), a gap acknowledged rather than
|
||||
closed here: narrowing this to exclude
|
||||
`POST/DELETE /api/teams/{teamID}/members*` and
|
||||
`.../invites*` specifically for a service-account caller is a small, isolated
|
||||
follow-up (special-case those handlers rather than `requireTeamOwner` itself,
|
||||
which every other owner-gated endpoint still wants shared). Until then, what
|
||||
actually keeps membership out of automation's hands is that no operator built
|
||||
against this scope should ever call those two endpoints — not a server-side
|
||||
refusal.
|
||||
`internal/api/middleware.go`'s dual resolution (`Authorization: Bearer` →
|
||||
`apiKeyUser()`, or session cookie → `sessionUser()`) and the service-account
|
||||
path (`serviceAccountFor()`) both resolve into one `Caller` type
|
||||
(`internal/api/caller.go`), not two parallel, un-unified context
|
||||
representations the way an earlier version of this server kept them. Every
|
||||
authorization predicate reads `Caller`'s methods:
|
||||
|
||||
- `Caller.IsAdmin()` — true **only** for a human system administrator, never
|
||||
for a service account of either scope, under any circumstance. `AdminOnly`
|
||||
and `requireSelfOrAdmin` key on this alone — user management
|
||||
(`POST /api/users`, `PUT /api/users/{id}/admin`, etc.) and
|
||||
`GET/PUT /api/admin/settings` stay human-only, forever. The original text
|
||||
here claimed an instance-scoped service account satisfies `AdminOnly` "for
|
||||
team-creation/listing purposes" — that was never true of the shipped code
|
||||
(`TEAM-LOOKUP.md` caught the listing half; the creation half was always a
|
||||
separate, bespoke check in `handleCreateTeam`, not `AdminOnly` itself) and
|
||||
is not being made true now. Don't widen `AdminOnly`: every time this has
|
||||
come up, the fix has been a narrower, purpose-built capability instead
|
||||
(`?name=` lookups for teams and service accounts; now
|
||||
`terdut-operator`'s own invite-minting feature for the one real gap this
|
||||
boundary left — how a human ever gets a first login on a no-OIDC,
|
||||
operator-managed install. See the bottom of "What this unblocks.")
|
||||
- `Caller.IsInstanceServiceAccount()` — true only for an instance-scoped
|
||||
service account, never for a human (including a human admin).
|
||||
`handleCreateTeam` uses exactly this: a human creates a team by being a
|
||||
human (and becomes its owner); an instance-scoped service account creates
|
||||
one with no human owner at all. The two paths are not interchangeable, so
|
||||
this predicate deliberately does not also admit a human admin.
|
||||
- `Caller.Role(teamID)`/`TeamIDs()` — a human's real `team_members` rows, or
|
||||
a team-scoped service account's single synthetic owner membership
|
||||
(`serveAsServiceAccount`). This is what makes `requireTeamMember`/
|
||||
`requireTeamOwner` treat a team-scoped service account as owner-equivalent
|
||||
for that one team, with no separate branch needed in either function.
|
||||
- `Caller.ServiceAccountID()` — used by `OperatorModeBlock` ("any service
|
||||
account passes") and by `callerMayManageServiceAccount`'s self-rotation
|
||||
check.
|
||||
- `Caller.AsHuman()` — the accessor every handler that needs a real
|
||||
`user_id` to act on behalf of must call and check, instead of reading a
|
||||
user off context unconditionally. Before the `Caller` type existed, four
|
||||
handlers did the latter and silently misbehaved for a service-account
|
||||
caller: `handleMe` and `handleTestNotification` 500'd (a zero-value user id
|
||||
that matches no row), `handleDismissOnboarding` silently no-op'd (`UPDATE
|
||||
... WHERE id = 0` affects nothing, still returns 204), and
|
||||
`handleCreateInvite` wrote that same zero value into `invites.created_by`
|
||||
— a real foreign-key violation, not just a wrong answer, since that column
|
||||
is nullable but was never passed as `nil`. All four now call `AsHuman()`
|
||||
and return an explicit 403 ("this endpoint is for human accounts only")
|
||||
or, for the invite case, leave `created_by` `NULL` the same way
|
||||
`handleCreateServiceAccount` already did for the analogous situation.
|
||||
|
||||
**Team scope is owner-equivalent for every `requireTeamOwner` endpoint,
|
||||
membership and invites included — by design, not by an unclosed gap.** An
|
||||
earlier version of this document flagged this as "acknowledged rather than
|
||||
closed," kept in check only by the social convention that nobody *builds*
|
||||
automation against those two routes. That convention is retired:
|
||||
`terdut-operator`'s `TerdutTeam` controller now mints and revokes its own
|
||||
team's invite link through exactly this capability (its existing
|
||||
team-scoped credential, `POST`/`DELETE /api/teams/{teamID}/invites`), which
|
||||
is the real fix for the human-onboarding gap below — not a narrower
|
||||
carve-out of this capability. `service_accounts_test.go`'s
|
||||
`TestServiceAccount_TeamScopeManagesItsOwnInvites` pins it.
|
||||
|
||||
**A team-scoped account can also mint another service account scoped to its
|
||||
own team** (`handleCreateServiceAccount`'s `callerOwnsTeam` branch, which a
|
||||
team-scoped caller already satisfies for its own team via the synthetic
|
||||
membership above). Kept, not restricted, for the same reason: a team-scoped
|
||||
credential is that team's owner's reach, full stop — carving this one
|
||||
capability out while leaving membership/invites alone would be an arbitrary
|
||||
asymmetry. Pinned by
|
||||
`TestServiceAccount_TeamScopeCanMintAnotherAccountForItsOwnTeam`.
|
||||
|
||||
**`callerMayManageServiceAccount` gained the one load-bearing fix this
|
||||
redesign exists for:** an instance-scoped service account may manage
|
||||
(mint/revoke a key on) *any* team-scoped account, not only one admin, that
|
||||
team's human owner, or the account itself. `handleCreateServiceAccount`
|
||||
already let an instance-scoped caller *create* a team-scoped account for
|
||||
any team; this closes the gap where adopting or rotating one it didn't just
|
||||
create in the same call — exactly `terdut-operator`'s documented
|
||||
adopt-on-409 crash-window recovery (its own `DESIGN.md` §5) — 403'd forever
|
||||
instead of succeeding (`terdut-operator#3`). Pinned by
|
||||
`TestServiceAccount_InstanceScopeAdoptsAnExistingTeamScopedAccountsKey`.
|
||||
|
||||
Anywhere identity is recorded for a human (incident timeline
|
||||
`acknowledged_by`/`assigned_to`, audit-relevant fields), a service-account
|
||||
caller is still coerced into a bare `user_id` of `0` today — `Caller`'s new
|
||||
`Identity()` accessor exists for exactly this follow-up, but wiring it in
|
||||
needs a schema migration (an actor-attribution column distinct from
|
||||
`user_id`) and is deliberately out of scope here. Tracked separately, not by
|
||||
this document.
|
||||
|
||||
## What this unblocks
|
||||
|
||||
@@ -177,6 +239,21 @@ Directly resolves `terdut-operator` DESIGN.md §6's two broken assumptions:
|
||||
holding many credentials in one place (the operator's namespace) an
|
||||
acceptable trade rather than reintroducing the mirrored design's
|
||||
server-admin-equivalent-everywhere problem.
|
||||
4. **A human can get a first login on a no-OIDC, operator-managed install —
|
||||
without ever touching `AdminOnly` or `/api/admin/settings`.** This was
|
||||
filed as `terdut-server#23` ("no API path to create a human login after
|
||||
bootstrap") and diagnosed, at the time, as this server needing to let a
|
||||
service account through `AdminOnly`. It doesn't: the fix lives entirely
|
||||
in `terdut-operator`, because a team-scoped credential was *already*
|
||||
owner-equivalent for `POST /api/teams/{teamID}/invites`, and invite
|
||||
redemption (`POST /api/signup` with an `invite` token) bypasses
|
||||
`signup_mode` entirely — `terdut-operator` just never grew a feature to
|
||||
use either fact. Its `TerdutTeam` controller now mints and surfaces one
|
||||
via its own existing team-scoped credential (`spec.invite`,
|
||||
`status.inviteSecretRef`, see that repo's own docs), so a human joins a
|
||||
CRD-managed team by a real invite link, the same way anyone else would.
|
||||
`terdut-server#23` is closed with this note once that feature ships — its
|
||||
named routes stay human-only, correctly, not a gap.
|
||||
|
||||
## Suggested sequencing
|
||||
|
||||
|
||||
@@ -15,5 +15,5 @@ type: application
|
||||
# appVersion and image.tag in values.yaml no longer agree, and that is not an oversight:
|
||||
# image.tag stays "latest", which is what a local install actually pulls. appVersion is
|
||||
# metadata and drives nothing.
|
||||
version: 0.33.1
|
||||
appVersion: "v0.33.1"
|
||||
version: 0.34.0
|
||||
appVersion: "v0.34.0"
|
||||
|
||||
@@ -21,6 +21,23 @@ spec:
|
||||
{{- include "terdut-server.selectorLabels" . | nindent 8 }}
|
||||
spec:
|
||||
enableServiceLinks: false
|
||||
{{- if .Values.database.waitForPostgres.enabled }}
|
||||
initContainers:
|
||||
- name: wait-for-postgres
|
||||
image: "{{ .Values.database.waitForPostgres.image.repository }}:{{ .Values.database.waitForPostgres.image.tag }}"
|
||||
imagePullPolicy: {{ .Values.database.waitForPostgres.image.pullPolicy }}
|
||||
env:
|
||||
- name: TERDUT_DB_DSN
|
||||
value: {{ required "database.dsn is required" .Values.database.dsn | quote }}
|
||||
command:
|
||||
- sh
|
||||
- -c
|
||||
- |
|
||||
until pg_isready -d "$TERDUT_DB_DSN"; do
|
||||
echo "wait-for-postgres: not ready yet, retrying in 2s"
|
||||
sleep 2
|
||||
done
|
||||
{{- end }}
|
||||
containers:
|
||||
- name: terdut-server
|
||||
image: "{{ .Values.image.repository }}:{{ .Values.image.tag }}"
|
||||
|
||||
@@ -33,6 +33,26 @@ database:
|
||||
passwordSecret:
|
||||
name: ""
|
||||
key: password
|
||||
# Blocks the main container from starting until Postgres accepts
|
||||
# connections. Without this, a Deployment created before Postgres has
|
||||
# finished its very first boot -- initdb plus Patroni leader election, on a
|
||||
# from-scratch postgres-operator cluster -- crash-loops a few times: the
|
||||
# app's own ping-retry budget on startup (pingAttempts/pingRetryDelay in
|
||||
# internal/db/db.go) is sized for a much shorter, different race --
|
||||
# NetworkPolicy propagation, a few seconds -- not for genuine first-time
|
||||
# cluster creation, which routinely takes longer, so it exhausts and the
|
||||
# process exits before ever binding its HTTP port. A startupProbe cannot
|
||||
# help here: the crash happens before there is anything to probe.
|
||||
#
|
||||
# pg_isready needs no credentials -- it reports PQPING_OK on anything that
|
||||
# amounts to "a Postgres backend answered", including an auth challenge --
|
||||
# so no PGPASSWORD is wired into this container.
|
||||
waitForPostgres:
|
||||
enabled: true
|
||||
image:
|
||||
repository: postgres
|
||||
tag: "17-alpine"
|
||||
pullPolicy: IfNotPresent
|
||||
|
||||
service:
|
||||
type: ClusterIP
|
||||
|
||||
@@ -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"))
|
||||
|
||||
@@ -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
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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)
|
||||
|
||||
+24
-4
@@ -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 {
|
||||
|
||||
@@ -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()
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user