internal/api: unify human/service-account authz into one Caller type #24
Assignees
niklas (Niklas Ye)
Clear assignees
niklas (Niklas Ye)
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: niklas/terdut-server#24
Reference in New Issue
Block a user
Delete Branch "unify-caller-authz"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Redesigns the authorization boundary between a human caller and a service account, instead of patching around it a third time. Closes #23 (by reframing it — see below) and
niklas/terdut-operator#3.Why
ctxUser/ctxTeams(human) andctxServiceAccount(+ a syntheticctxTeamsentry, service account) were two parallel, un-unified context representations. Every authz predicate had to remember which one(s) it needed to check, and every place that forgot either wrongly 403'd a service account (#23,niklas/terdut-operator#3), crashed on an unchecked zero-value user id, or silently no-op'd.SERVICE-ACCOUNTS.mdandTEAM-LOOKUP.mdhad already each independently caught one instance of this; auditing the whole package found several more.What changed
New
internal/api/caller.gocollapses both representations into oneCaller, stored under onectxCallerkey byserveAs/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 restructuring:
callerMayManageServiceAccountgains the one load-bearing branch this PR exists for: an instance-scoped service account may now manage (mint/revoke a key on) any team-scoped account, not only a human admin, that team's own human owner, or the account itself.handleCreateServiceAccountalready let an instance-scoped caller create one for any team; adopting/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. Closesniklas/terdut-operator#3.handleCreateInvitewrote a service-account caller's zero-value user id straight intoinvites.created_by(nullable, but never passed asnil) — a foreign-key-violation 500, not success, for any team-scoped service account minting an invite. Fixed the same wayhandleCreateServiceAccountalready handles the analogous case.handleMeandhandleTestNotification500'd for a service-account caller;handleDismissOnboardingsilently no-op'd. All three now return an explicit 403 ("this endpoint is for human accounts only").terdut-operator's newTerdutTeaminvite-minting feature (follow-up PR) depends on the first one.AdminOnly/requireSelfOrAdminare unchanged in effect — still human-only, forever, for every scope of service account (TestAdminOnly_RefusesEveryServiceAccountScope). #23's named routes (POST /api/users,PUT /api/admin/settings) were never the right thing to widen; its real fix is entirely on theterdut-operatorside (a team-scoped credential is already owner-equivalent for minting a team invite, and invite redemption bypassessignup_modeentirely — it just never grew a feature to use either fact). Recorded inSERVICE-ACCOUNTS.md's "What this unblocks"; closing #23 once that feature ships.SERVICE-ACCOUNTS.mdamended in place (its own established convention) to describe the as-builtCallermodel and correct its own aspirational claim aboutAdminOnlythatTEAM-LOOKUP.mdhad already flagged as not matching shipped code.Explicitly out of scope
A separate, real bug the same audit found: every incident-mutation route (acknowledge/resolve/assign/snooze/archive/notes) also writes a service account's zero-value user id into
acknowledged_by/assigned_to/incident_events.user_id— a 500, same shape. Fixing that needs a schema migration (an actor-attribution column distinct fromuser_id) and is real feature work (can a service account act on incidents on a team's behalf at all), not an authorization bug. Will file separately.Testing
make fmt lint testgreen, including new coverage:TestServiceAccount_InstanceScopeAdoptsAnExistingTeamScopedAccountsKey— the #operator-3 regression test.TestServiceAccount_TeamScopeCanMintAnotherAccountForItsOwnTeamTestServiceAccount_TeamScopeManagesItsOwnInvitesTestSignup_InviteCreatedByAServiceAccountLeavesCreatedByNullTestServiceAccount_HumanOnlyEndpointsRefuseExplicitlyTestAdminOnly_RefusesEveryServiceAccountScopeEvery existing test (
admin_test.go,settings_test.go,TestOperatorMode_*,TestListTeamsByName_*, the full incident/alert/schedule/stats suites) passes unchanged, confirming the refactor didn't perturb existing behavior.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.