diff --git a/internal/api/alertmanager.go b/internal/api/alertmanager.go index 012513f..0a41784 100644 --- a/internal/api/alertmanager.go +++ b/internal/api/alertmanager.go @@ -169,7 +169,7 @@ func ingest(ctx context.Context, db *sql.DB, notify NotifyConfig, src alertSourc } touched[id] = true alertID := a.id - if err := logEvent(ctx, tx, id, evAlertResolved, nil, &alertID, nil); err != nil { + if err := logEvent(ctx, tx, id, evAlertResolved, nil, nil, &alertID, nil); err != nil { return err } } @@ -417,12 +417,12 @@ func openIncident(ctx context.Context, q querier, notify NotifyConfig, teamID in return 0, err } - if err := logEvent(ctx, q, id, evTriggered, nil, nil, nil); err != nil { + if err := logEvent(ctx, q, id, evTriggered, nil, nil, nil, nil); err != nil { return 0, err } if onCall != nil { // On an "assigned" event user_id is the assignee, not the actor. - if err := logEvent(ctx, q, id, evAssigned, onCall, nil, nil); err != nil { + if err := logEvent(ctx, q, id, evAssigned, onCall, nil, nil, nil); err != nil { return 0, err } } @@ -470,5 +470,5 @@ func linkAlert(ctx context.Context, tx *sql.Tx, incidentID, alertID int64) error if n, _ := res.RowsAffected(); n == 0 { return nil } - return logEvent(ctx, tx, incidentID, evAlertAdded, nil, &alertID, nil) + return logEvent(ctx, tx, incidentID, evAlertAdded, nil, nil, &alertID, nil) } diff --git a/internal/api/archiver.go b/internal/api/archiver.go index d2bb24d..c014bae 100644 --- a/internal/api/archiver.go +++ b/internal/api/archiver.go @@ -142,7 +142,7 @@ func expireStale(ctx context.Context, db *sql.DB, staleAfter time.Duration, skip continue } alertID := id - if err := logEvent(ctx, db, incidentID, evAlertResolved, nil, &alertID, nil); err != nil { + if err := logEvent(ctx, db, incidentID, evAlertResolved, nil, nil, &alertID, nil); err != nil { log.Printf("sweeper: log expiry event: %v", err) } } diff --git a/internal/api/caller.go b/internal/api/caller.go index 3370870..95501de 100644 --- a/internal/api/caller.go +++ b/internal/api/caller.go @@ -99,13 +99,24 @@ func (c Caller) ServiceAccountID() (int64, bool) { return c.sa.id, true } +// ServiceAccountName reports this caller's own service-account name, for a +// handler's synchronous response — the same credential it authenticated +// with, already resolved onto the Caller by serveAsServiceAccount, so no +// extra query is needed. +func (c Caller) ServiceAccountName() (string, bool) { + if c.sa == nil { + return "", false + } + return c.sa.name, 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. +// wired into any database column — incidents.go's acknowledged_by/ +// incident_events.user_id use AsHuman()/ServiceAccountID() directly against +// the parallel *_service_account_id columns (migration 015) instead, since a +// column needs the id, not this rendered string. assigned_to stays +// human-only and out of scope (terdut-server#25's follow-up). func (c Caller) Identity() string { switch { case c.user != nil: diff --git a/internal/api/deadman.go b/internal/api/deadman.go index 441c0fa..c342993 100644 --- a/internal/api/deadman.go +++ b/internal/api/deadman.go @@ -373,7 +373,7 @@ func deadmanDied(ctx context.Context, db *sql.DB, notify NotifyConfig, hb deadma alertID := hb.id detail := "last heartbeat " + humanDuration(now.Sub(time.Unix(hb.receivedAt, 0))) + " ago" - if err := logEvent(ctx, tx, incidentID, evDeadmanSilent, nil, &alertID, &detail); err != nil { + if err := logEvent(ctx, tx, incidentID, evDeadmanSilent, nil, nil, &alertID, &detail); err != nil { return err } @@ -415,7 +415,7 @@ func deadmanRecovered(ctx context.Context, db *sql.DB, hb deadmanAlert) error { time.Now().Unix(), incidentResolutionRecovered, incidentID); err != nil { return err } - if err := logEvent(ctx, tx, incidentID, evResolved, nil, nil, nil); err != nil { + if err := logEvent(ctx, tx, incidentID, evResolved, nil, nil, nil, nil); err != nil { return err } // The all-clear goes to whoever was paged, which enqueueResolved works out diff --git a/internal/api/escalation.go b/internal/api/escalation.go index 2fdf28e..59c3a84 100644 --- a/internal/api/escalation.go +++ b/internal/api/escalation.go @@ -229,7 +229,7 @@ func advanceEscalation(ctx context.Context, db *sql.DB, cfg NotifyConfig, policy // nobody. That is a policy that looks configured and is not. detail += ": nobody reachable" } - if err := logEvent(ctx, tx, incidentID, evEscalated, nil, nil, &detail); err != nil { + if err := logEvent(ctx, tx, incidentID, evEscalated, nil, nil, nil, &detail); err != nil { return err } return tx.Commit() @@ -256,7 +256,7 @@ func escalationExhausted(ctx context.Context, tx *sql.Tx, policy *escalationPoli incidentID); err != nil { return err } - return logEvent(ctx, tx, incidentID, evEscalated, nil, nil, &detail) + return logEvent(ctx, tx, incidentID, evEscalated, nil, nil, nil, &detail) } // pageLevel notifies every target of one level and reports who was woken. diff --git a/internal/api/incident_store.go b/internal/api/incident_store.go index 29f4802..6fede5d 100644 --- a/internal/api/incident_store.go +++ b/internal/api/incident_store.go @@ -65,11 +65,13 @@ const incidentSelectFrom = ` WHERE el.team_id = i.team_id AND el.position = i.escalation_level), i.triggered_at, i.acknowledged_by, i.acknowledged_at, ack.username, + i.acknowledged_by_service_account_id, acksa.name, i.assigned_to, asg.username, i.snoozed_until, i.resolved_at, i.resolution_source, i.archived_at FROM incidents i JOIN teams t ON t.id = i.team_id LEFT JOIN users ack ON ack.id = i.acknowledged_by + LEFT JOIN service_accounts acksa ON acksa.id = i.acknowledged_by_service_account_id LEFT JOIN users asg ON asg.id = i.assigned_to` func scanIncident(s scanner) (models.Incident, error) { @@ -83,6 +85,7 @@ func scanIncident(s scanner) (models.Incident, error) { &i.EscalationLevel, &escalationDue, &triggeredAt, &i.AcknowledgedByID, &ackAt, &i.AcknowledgedByUser, + &i.AcknowledgedByServiceAccountID, &i.AcknowledgedByServiceAccountName, &i.AssignedToID, &i.AssignedToUser, &snoozedUntil, &resolvedAt, &i.ResolutionSource, &archivedAt, ); err != nil { @@ -112,13 +115,31 @@ func fetchIncident(ctx context.Context, q querier, id int64) (models.Incident, e return scanIncident(q.QueryRowContext(ctx, incidentSelectFrom+" WHERE i.id = $1", id)) } -// logEvent appends one entry to an incident's timeline. A nil userID means the -// server acted rather than a person. -func logEvent(ctx context.Context, q querier, incidentID int64, evType string, userID, alertID *int64, detail *string) error { +// callerActorIDs resolves the current request's caller into the pair of +// nilable ids logEvent/acknowledgeIncidentAs expect: exactly one of userID/ +// serviceAccountID is set (never both), replacing the unchecked +// userFromContext(ctx) zero-value reads that used to write a human-only id +// of 0 for a service-account caller (terdut-server#25). +func callerActorIDs(ctx context.Context) (userID, serviceAccountID *int64) { + caller, _ := callerFromContext(ctx) + if u, ok := caller.AsHuman(); ok { + return &u.ID, nil + } + if id, ok := caller.ServiceAccountID(); ok { + return nil, &id + } + return nil, nil +} + +// logEvent appends one entry to an incident's timeline. userID and +// serviceAccountID are mutually exclusive and both nilable; both nil means +// the server acted rather than any caller (see incident_events_actor_xor_chk, +// migration 015). +func logEvent(ctx context.Context, q querier, incidentID int64, evType string, userID, serviceAccountID, alertID *int64, detail *string) error { _, err := q.ExecContext(ctx, ` - INSERT INTO incident_events (incident_id, type, user_id, alert_id, detail, created_at) - VALUES ($1, $2, $3, $4, $5, $6)`, - incidentID, evType, userID, alertID, detail, time.Now().Unix()) + INSERT INTO incident_events (incident_id, type, user_id, service_account_id, alert_id, detail, created_at) + VALUES ($1, $2, $3, $4, $5, $6, $7)`, + incidentID, evType, userID, serviceAccountID, alertID, detail, time.Now().Unix()) return err } @@ -251,7 +272,7 @@ func resolveIfSettled(ctx context.Context, q querier, incidentID int64) (bool, e if err := stopEscalation(ctx, q, incidentID); err != nil { return false, err } - if err := logEvent(ctx, q, incidentID, evResolved, nil, nil, nil); err != nil { + if err := logEvent(ctx, q, incidentID, evResolved, nil, nil, nil, nil); err != nil { return false, err } // The all-clear goes only to whoever was paged in the first place, which @@ -260,19 +281,30 @@ func resolveIfSettled(ctx context.Context, q querier, incidentID int64) (bool, e return true, enqueueResolved(ctx, q, incidentID) } -// acknowledgeIncident records that userID has picked an incident up, and reports -// whether it changed anything — an already-resolved or already-acknowledged -// incident is left alone, so a second acknowledge (a retried request, or a -// stale push notification tapped after the web UI already acked it) is a -// no-op rather than a second "acknowledged" timeline entry. Shared by the -// authenticated handler and the Acknowledge button in a push notification, -// so both write the same state and the same timeline entry. +// acknowledgeIncident records that userID — a human — has picked an incident +// up, and reports whether it changed anything — an already-resolved or +// already-acknowledged incident is left alone, so a second acknowledge (a +// retried request, or a stale push notification tapped after the web UI +// already acked it) is a no-op rather than a second "acknowledged" timeline +// entry. Used only by the Acknowledge button in a push notification +// (notify_ack.go), which always resolves a human from +// incident_ack_tokens.user_id — there is no service-account equivalent of +// that flow, so this keeps its human-only signature; the authenticated +// handler goes through acknowledgeIncidentAs below instead. func acknowledgeIncident(ctx context.Context, q querier, incidentID, userID int64) (bool, error) { + return acknowledgeIncidentAs(ctx, q, incidentID, &userID, nil) +} + +// acknowledgeIncidentAs is acknowledgeIncident generalized to either actor +// kind. userID and serviceAccountID are mutually exclusive and nilable the +// same way logEvent's are (see incidents_ack_actor_xor_chk, migration 015). +func acknowledgeIncidentAs(ctx context.Context, q querier, incidentID int64, userID, serviceAccountID *int64) (bool, error) { res, err := q.ExecContext(ctx, ` UPDATE incidents - SET status = 'acknowledged', acknowledged_by = $1, acknowledged_at = $2 - WHERE id = $3 AND status = 'triggered'`, - userID, time.Now().Unix(), incidentID) + SET status = 'acknowledged', acknowledged_by = $1, acknowledged_by_service_account_id = $2, + acknowledged_at = $3 + WHERE id = $4 AND status = 'triggered'`, + userID, serviceAccountID, time.Now().Unix(), incidentID) if err != nil { return false, err } @@ -283,7 +315,7 @@ func acknowledgeIncident(ctx context.Context, q querier, incidentID, userID int6 if err := stopEscalation(ctx, q, incidentID); err != nil { return false, err } - return true, logEvent(ctx, q, incidentID, evAcknowledged, &userID, nil, nil) + return true, logEvent(ctx, q, incidentID, evAcknowledged, userID, serviceAccountID, nil, nil) } // openIncidentForAlert returns the open incident an alert currently belongs to, diff --git a/internal/api/incidents.go b/internal/api/incidents.go index 18a0c84..8714fdd 100644 --- a/internal/api/incidents.go +++ b/internal/api/incidents.go @@ -157,9 +157,11 @@ func handleIncidentTimeline(db *sql.DB) http.HandlerFunc { rows, err := db.QueryContext(r.Context(), ` SELECT e.id, e.incident_id, e.type, e.user_id, u.username, + e.service_account_id, sa.name, e.alert_id, e.detail, e.created_at FROM incident_events e LEFT JOIN users u ON u.id = e.user_id + LEFT JOIN service_accounts sa ON sa.id = e.service_account_id WHERE e.incident_id = $1 ORDER BY e.created_at ASC, e.id ASC`, id) if err != nil { @@ -173,6 +175,7 @@ func handleIncidentTimeline(db *sql.DB) http.HandlerFunc { var e models.IncidentEvent var ts int64 if err := rows.Scan(&e.ID, &e.IncidentID, &e.Type, &e.UserID, &e.Username, + &e.ServiceAccountID, &e.ServiceAccountName, &e.AlertID, &e.Detail, &ts); err != nil { respond(w, http.StatusInternalServerError, errResp("internal error")) return @@ -190,8 +193,8 @@ func handleIncidentAcknowledge(db *sql.DB) http.HandlerFunc { if !ok { return } - user, _ := userFromContext(r.Context()) - acked, err := acknowledgeIncident(r.Context(), db, id, user.ID) + userID, saID := callerActorIDs(r.Context()) + acked, err := acknowledgeIncidentAs(r.Context(), db, id, userID, saID) if err != nil { respond(w, http.StatusInternalServerError, errResp("internal error")) return @@ -223,13 +226,14 @@ func handleIncidentUnacknowledge(db *sql.DB) http.HandlerFunc { if !ok { return } - user, _ := userFromContext(r.Context()) + userID, saID := callerActorIDs(r.Context()) if !updateOpenIncident(w, r, db, id, - `UPDATE incidents SET status = 'triggered', acknowledged_by = NULL, acknowledged_at = NULL + `UPDATE incidents SET status = 'triggered', acknowledged_by = NULL, + acknowledged_by_service_account_id = NULL, acknowledged_at = NULL WHERE id = $1 AND resolved_at IS NULL`, id) { return } - if err := logEvent(r.Context(), db, id, evUnacknowledged, &user.ID, nil, nil); err != nil { + if err := logEvent(r.Context(), db, id, evUnacknowledged, userID, saID, nil, nil); err != nil { respond(w, http.StatusInternalServerError, errResp("internal error")) return } @@ -247,7 +251,7 @@ func handleIncidentResolve(db *sql.DB) http.HandlerFunc { if !ok { return } - user, _ := userFromContext(r.Context()) + userID, saID := callerActorIDs(r.Context()) // The body is optional: clients that predate resolution notes send none. var req struct { Resolution string `json:"resolution"` @@ -268,12 +272,12 @@ func handleIncidentResolve(db *sql.DB) http.HandlerFunc { respond(w, http.StatusInternalServerError, errResp("internal error")) return } - if err := logEvent(r.Context(), db, id, evResolved, &user.ID, nil, nil); err != nil { + if err := logEvent(r.Context(), db, id, evResolved, userID, saID, nil, nil); err != nil { respond(w, http.StatusInternalServerError, errResp("internal error")) return } if req.Resolution != "" { - if err := logEvent(r.Context(), db, id, evResolutionNote, &user.ID, nil, &req.Resolution); err != nil { + if err := logEvent(r.Context(), db, id, evResolutionNote, userID, saID, nil, &req.Resolution); err != nil { respond(w, http.StatusInternalServerError, errResp("internal error")) return } @@ -312,7 +316,7 @@ func handleIncidentAssign(db *sql.DB) http.HandlerFunc { return } // On an "assigned" event user_id is the assignee, not the actor. - if err := logEvent(r.Context(), db, id, evAssigned, &req.UserID, nil, nil); err != nil { + if err := logEvent(r.Context(), db, id, evAssigned, &req.UserID, nil, nil, nil); err != nil { respond(w, http.StatusInternalServerError, errResp("internal error")) return } @@ -363,14 +367,14 @@ func handleIncidentSnooze(db *sql.DB) http.HandlerFunc { return } - user, _ := userFromContext(r.Context()) + userID, saID := callerActorIDs(r.Context()) if !updateOpenIncident(w, r, db, id, "UPDATE incidents SET snoozed_until = $1 WHERE id = $2 AND resolved_at IS NULL", until.Unix(), id) { return } detail := until.UTC().Format(time.RFC3339) - if err := logEvent(r.Context(), db, id, evSnoozed, &user.ID, nil, &detail); err != nil { + if err := logEvent(r.Context(), db, id, evSnoozed, userID, saID, nil, &detail); err != nil { respond(w, http.StatusInternalServerError, errResp("internal error")) return } @@ -384,12 +388,12 @@ func handleIncidentUnsnooze(db *sql.DB) http.HandlerFunc { if !ok { return } - user, _ := userFromContext(r.Context()) + userID, saID := callerActorIDs(r.Context()) if !updateOpenIncident(w, r, db, id, "UPDATE incidents SET snoozed_until = NULL WHERE id = $1 AND resolved_at IS NULL", id) { return } - if err := logEvent(r.Context(), db, id, evUnsnoozed, &user.ID, nil, nil); err != nil { + if err := logEvent(r.Context(), db, id, evUnsnoozed, userID, saID, nil, nil); err != nil { respond(w, http.StatusInternalServerError, errResp("internal error")) return } @@ -466,27 +470,32 @@ func handleCreateNote(db *sql.DB) http.HandlerFunc { return } - user, _ := userFromContext(r.Context()) + caller, _ := callerFromContext(r.Context()) + userID, saID := callerActorIDs(r.Context()) now := time.Now() var eventID int64 err := db.QueryRowContext(r.Context(), ` - INSERT INTO incident_events (incident_id, type, user_id, detail, created_at) - VALUES ($1, $2, $3, $4, $5) - RETURNING id`, id, noteType, user.ID, req.Content, now.Unix()).Scan(&eventID) + INSERT INTO incident_events (incident_id, type, user_id, service_account_id, detail, created_at) + VALUES ($1, $2, $3, $4, $5, $6) + RETURNING id`, id, noteType, userID, saID, req.Content, now.Unix()).Scan(&eventID) if err != nil { respond(w, http.StatusInternalServerError, errResp("internal error")) return } - respond(w, http.StatusCreated, models.IncidentEvent{ + resp := models.IncidentEvent{ ID: eventID, IncidentID: id, Type: noteType, - UserID: &user.ID, - Username: &user.Username, Detail: &req.Content, CreatedAt: now.UTC().Truncate(time.Second), - }) + } + if u, ok := caller.AsHuman(); ok { + resp.UserID, resp.Username = &u.ID, &u.Username + } else if saName, ok := caller.ServiceAccountName(); ok { + resp.ServiceAccountID, resp.ServiceAccountName = saID, &saName + } + respond(w, http.StatusCreated, resp) } } @@ -504,11 +513,12 @@ func handleDeleteNote(db *sql.DB) http.HandlerFunc { return } - user, _ := userFromContext(r.Context()) + userID, saID := callerActorIDs(r.Context()) res, err := db.ExecContext(r.Context(), ` DELETE FROM incident_events - WHERE id = $1 AND incident_id = $2 AND type IN ($3, $4) AND user_id = $5`, - eventID, id, evNote, evResolutionNote, user.ID) + WHERE id = $1 AND incident_id = $2 AND type IN ($3, $4) + AND (user_id = $5 OR service_account_id = $6)`, + eventID, id, evNote, evResolutionNote, userID, saID) if err != nil { respond(w, http.StatusInternalServerError, errResp("internal error")) return diff --git a/internal/api/incidents_test.go b/internal/api/incidents_test.go index f55f6e2..98c0192 100644 --- a/internal/api/incidents_test.go +++ b/internal/api/incidents_test.go @@ -8,6 +8,7 @@ import ( "time" "git.ryuvia.com/niklas/terdut-server/internal/api" + "git.ryuvia.com/niklas/terdut-server/internal/models" ) // amAlert builds one alert of a webhook payload. @@ -642,6 +643,106 @@ func TestIncident_ArchiveRoundTrip(t *testing.T) { } } +// --------------------------------------------------------------------------- +// Service accounts (terdut-server#25) +// --------------------------------------------------------------------------- + +// TestServiceAccount_CanActOnItsTeamsIncidents is #25's regression test. +// Before the fix: acknowledge/resolve/snooze/create-note each 500'd (writing +// acknowledged_by/user_id = 0, violating the users(id) FK for a service +// account), and delete-note silently matched zero rows (WHERE user_id = 0) +// instead of deleting. +func TestServiceAccount_CanActOnItsTeamsIncidents(t *testing.T) { + s := newTS(t) + instanceKey := createServiceAccount(t, s, s.key, "operator", models.ServiceAccountScopeInstance, 0) + teamA := createTeamAs(t, s, instanceKey, "team-a") + keyA := createServiceAccount(t, s, instanceKey, "team-a-sa", models.ServiceAccountScopeTeam, teamA) + + var integration struct { + Key string `json:"key"` + } + decode(t, s.reqAs(t, keyA, http.MethodPost, "/api/teams/"+id64(teamA)+"/integrations", + map[string]string{"name": "test"}), &integration) + postToIntegration(t, s, integration.Key, "fp-sa", "SAIncident") // incident 1 + + // Acknowledge. + resp := s.reqAs(t, keyA, http.MethodPost, "/api/incidents/1/acknowledge", nil) + if resp.StatusCode != http.StatusOK { + t.Fatalf("service account acknowledge: %d", resp.StatusCode) + } + var inc map[string]any + decode(t, resp, &inc) + if inc["acknowledged_by_service_account_id"] == nil { + t.Error("expected acknowledged_by_service_account_id to be set") + } + if inc["acknowledged_by_id"] != nil { + t.Errorf("expected acknowledged_by_id to stay nil for a service-account actor, got %v", inc["acknowledged_by_id"]) + } + + // Unacknowledge. + resp = s.reqAs(t, keyA, http.MethodDelete, "/api/incidents/1/acknowledge", nil) + resp.Body.Close() + if resp.StatusCode != http.StatusNoContent { + t.Errorf("service account unacknowledge: %d", resp.StatusCode) + } + + // Snooze, then unsnooze. + resp = s.reqAs(t, keyA, http.MethodPost, "/api/incidents/1/snooze", + map[string]string{"duration": "1h"}) + resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Errorf("service account snooze: %d", resp.StatusCode) + } + resp = s.reqAs(t, keyA, http.MethodDelete, "/api/incidents/1/snooze", nil) + resp.Body.Close() + if resp.StatusCode != http.StatusNoContent { + t.Errorf("service account unsnooze: %d", resp.StatusCode) + } + + // Create, then delete, a note. + var note map[string]any + decode(t, s.reqAs(t, keyA, http.MethodPost, "/api/incidents/1/notes", + map[string]string{"content": "looking into it"}), ¬e) + if note["service_account_id"] == nil { + t.Error("expected service_account_id on the note event") + } + if note["user_id"] != nil { + t.Errorf("expected no user_id on a service-account note, got %v", note["user_id"]) + } + noteID := int(note["id"].(float64)) + delResp := s.reqAs(t, keyA, http.MethodDelete, fmt.Sprintf("/api/incidents/1/notes/%d", noteID), nil) + delResp.Body.Close() + if delResp.StatusCode != http.StatusNoContent { + t.Errorf("service account deleting its own note: %d", delResp.StatusCode) + } + + // Resolve. + resp = s.reqAs(t, keyA, http.MethodPost, "/api/incidents/1/resolve", nil) + resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Errorf("service account resolve: %d", resp.StatusCode) + } +} + +// Regression guard: a human actor must still write only the human columns, +// unaffected by the service-account branch added above. +func TestIncident_AcknowledgeStillWritesOnlyHumanColumn(t *testing.T) { + s := newTS(t) + postWebhook(t, s, []map[string]any{ + amAlert("fp-human-ack", "Z", "firing", "2026-05-20T10:00:00Z", zeroTime, nil), + }) + + var inc map[string]any + decode(t, s.req(t, http.MethodPost, "/api/incidents/1/acknowledge", nil), &inc) + if inc["acknowledged_by_id"] == nil { + t.Error("expected acknowledged_by_id to be set for a human actor") + } + if inc["acknowledged_by_service_account_id"] != nil { + t.Errorf("expected acknowledged_by_service_account_id to stay nil for a human actor, got %v", + inc["acknowledged_by_service_account_id"]) + } +} + func TestSweeper_ArchivesResolvedIncidents(t *testing.T) { s := newTS(t) postWebhook(t, s, []map[string]any{ diff --git a/internal/api/notifier.go b/internal/api/notifier.go index f34cb03..66a46a9 100644 --- a/internal/api/notifier.go +++ b/internal/api/notifier.go @@ -257,7 +257,7 @@ func deliverPending(ctx context.Context, db *sql.DB, cfg NotifyConfig) { } // Logged, not returned: the page has already gone out, and treating a // failed timeline write as a failed delivery would send it again. - if err := logEvent(ctx, db, n.incidentID, eventNotified, n.userID, nil, &n.kind); err != nil { + if err := logEvent(ctx, db, n.incidentID, eventNotified, n.userID, nil, nil, &n.kind); err != nil { log.Printf("notifier: log delivery of %d: %v", n.id, err) } sent++ @@ -312,7 +312,7 @@ func markFailed(ctx context.Context, db *sql.DB, n outboxRow, cause error) { return } detail := fmt.Sprintf("%s: %s", n.kind, cause) - if err := logEvent(ctx, db, n.incidentID, eventNotifyFailed, n.userID, nil, &detail); err != nil { + if err := logEvent(ctx, db, n.incidentID, eventNotifyFailed, n.userID, nil, nil, &detail); err != nil { log.Printf("notifier: log failure of %d: %v", n.id, err) } } diff --git a/internal/db/migrations/015_incident_service_account_actors.sql b/internal/db/migrations/015_incident_service_account_actors.sql new file mode 100644 index 0000000..b225f34 --- /dev/null +++ b/internal/db/migrations/015_incident_service_account_actors.sql @@ -0,0 +1,39 @@ +-- Service-account actors on incident mutations (terdut-server#25). A +-- team-scoped service account acknowledging/resolving/snoozing/noting an +-- incident is not a users row, so it cannot be written into +-- acknowledged_by/incident_events.user_id — doing so either violates the +-- users(id) FK (new rows) or, for incident_events.user_id, silently matches +-- zero rows on delete. These columns are the service-account-shaped parallel +-- to the existing human ones: nullable, mutually exclusive with their human +-- counterpart, ON DELETE SET NULL so a deleted service account doesn't take +-- the incident history with it. +ALTER TABLE incidents + ADD COLUMN acknowledged_by_service_account_id BIGINT + REFERENCES service_accounts(id) ON DELETE SET NULL; + +ALTER TABLE incident_events + ADD COLUMN service_account_id BIGINT + REFERENCES service_accounts(id) ON DELETE SET NULL; + +-- At most one actor kind per row: both NULL ("the server acted") is valid, +-- exactly one set is valid, both set is a bug this constraint refuses to +-- store rather than silently accepting. +ALTER TABLE incidents + ADD CONSTRAINT incidents_ack_actor_xor_chk CHECK ( + acknowledged_by IS NULL OR acknowledged_by_service_account_id IS NULL + ); + +ALTER TABLE incident_events + ADD CONSTRAINT incident_events_actor_xor_chk CHECK ( + user_id IS NULL OR service_account_id IS NULL + ); + +CREATE INDEX incidents_acknowledged_by_service_account_id_idx + ON incidents(acknowledged_by_service_account_id); +CREATE INDEX incident_events_service_account_id_idx + ON incident_events(service_account_id); + +-- assigned_to_service_account_id is deliberately not added here: it would sit +-- unpopulated until handleIncidentAssign itself tracks an actor, which is a +-- separate, pre-existing gap (it records the assignee today, never the +-- actor, for humans either) tracked in its own follow-up issue. diff --git a/internal/models/incident.go b/internal/models/incident.go index 9fe05bf..7ce9bbd 100644 --- a/internal/models/incident.go +++ b/internal/models/incident.go @@ -43,6 +43,13 @@ type Incident struct { AcknowledgedByUser *string `json:"acknowledged_by,omitempty"` AcknowledgedAt *time.Time `json:"acknowledged_at,omitempty"` + // AcknowledgedByServiceAccountID/Name are the service-account-shaped + // parallel to AcknowledgedByID/User above: mutually exclusive with it, + // populated when a service account (not a human) acknowledged this + // incident. See migration 015 and terdut-server#25. + AcknowledgedByServiceAccountID *int64 `json:"acknowledged_by_service_account_id,omitempty"` + AcknowledgedByServiceAccountName *string `json:"acknowledged_by_service_account,omitempty"` + AssignedToID *int64 `json:"assigned_to_id,omitempty"` AssignedToUser *string `json:"assigned_to,omitempty"` @@ -67,17 +74,27 @@ type Incident struct { // and is the only history this server keeps — alert rows are mutated in place. // // Type is one of: triggered, alert_added, alert_resolved, acknowledged, -// unacknowledged, assigned, snoozed, unsnoozed, resolved, note. A nil UserID -// means the server acted rather than a person. +// unacknowledged, assigned, snoozed, unsnoozed, resolved, note. UserID and +// ServiceAccountID are mutually exclusive; both nil means the server acted +// rather than any caller. type IncidentEvent struct { - ID int64 `json:"id"` - IncidentID int64 `json:"incident_id"` - Type string `json:"type"` - UserID *int64 `json:"user_id,omitempty"` - Username *string `json:"username,omitempty"` - AlertID *int64 `json:"alert_id,omitempty"` - Detail *string `json:"detail,omitempty"` - CreatedAt time.Time `json:"created_at"` + ID int64 `json:"id"` + IncidentID int64 `json:"incident_id"` + Type string `json:"type"` + UserID *int64 `json:"user_id,omitempty"` + Username *string `json:"username,omitempty"` + + // ServiceAccountID/Name are the service-account-shaped parallel to + // UserID/Username above: mutually exclusive with it, populated when a + // service account (not a human, and not nil-meaning-the-server-acted) + // performed this event. Named Name, not Username — a ServiceAccount has + // a Name field, not a Username. See migration 015 and terdut-server#25. + ServiceAccountID *int64 `json:"service_account_id,omitempty"` + ServiceAccountName *string `json:"service_account_name,omitempty"` + + AlertID *int64 `json:"alert_id,omitempty"` + Detail *string `json:"detail,omitempty"` + CreatedAt time.Time `json:"created_at"` } // SimilarIncident is an earlier, resolved incident with the same signature as diff --git a/internal/web/static/js/incident.js b/internal/web/static/js/incident.js index 97134f5..856d3fc 100644 --- a/internal/web/static/js/incident.js +++ b/internal/web/static/js/incident.js @@ -123,6 +123,16 @@ function who(id, name) { return name || 'someone'; } +// ackActorLabel renders whoever acknowledged inc, human or service account — +// the two are mutually exclusive (migration 015), and a service account is a +// credential, not "you" or "nobody", so it gets its own branch rather than +// going through who()'s id-vs-myID() check. +function ackActorLabel() { + if (inc.acknowledged_by_id != null) return who(inc.acknowledged_by_id, inc.acknowledged_by); + if (inc.acknowledged_by_service_account_id != null) return inc.acknowledged_by_service_account || 'a service account'; + return null; +} + function facts() { const rows = []; const add = (k, ...v) => rows.push(h('dt', { text: k }), h('dd', {}, ...v)); @@ -132,8 +142,7 @@ function facts() { // take reading top to bottom to piece together. const elapsedTo = inc.resolved_at ? Date.parse(inc.resolved_at) : Date.now(); const responsible = inc.assigned_to_id != null ? who(inc.assigned_to_id, inc.assigned_to) - : inc.acknowledged_by_id != null ? who(inc.acknowledged_by_id, inc.acknowledged_by) - : 'Unassigned'; + : ackActorLabel() || 'Unassigned'; rows.push(h('dt', { text: 'At a glance' }), h('dd', { class: 'fact-summary' }, h('span', { class: 'fact-chip' }, icon('clock', 'icon fact-icon'), duration(elapsedTo - Date.parse(inc.triggered_at))), inc.severity && badge(inc.severity, `plain ${severityClass(inc.severity)}`), @@ -142,7 +151,7 @@ function facts() { add('Triggered', when(inc.triggered_at), h('span', { class: 'sub', text: ` · ${ago(inc.triggered_at)}` })); if (inc.acknowledged_at) { - add('Acknowledged', `${who(inc.acknowledged_by_id, inc.acknowledged_by)} · ${when(inc.acknowledged_at)}`); + add('Acknowledged', `${ackActorLabel()} · ${when(inc.acknowledged_at)}`); } add('Assigned', inc.assigned_to_id != null ? who(inc.assigned_to_id, inc.assigned_to) : 'Unassigned'); if (inc.status !== 'resolved' && isFuture(inc.snoozed_until)) { @@ -206,9 +215,19 @@ function alertItem(a) { // ---------- timeline ---------- +// actorLabel renders whoever performed ev, human or service account — the +// two are mutually exclusive (migration 015). null means the server acted: +// ev.user_id == null no longer means that by itself, now that a service +// account's events also leave it null. +function actorLabel(ev, named = false) { + if (ev.user_id != null) return named ? (ev.username || 'someone') : who(ev.user_id, ev.username); + if (ev.service_account_id != null) return ev.service_account_name || 'a service account'; + return null; +} + // named spells users out instead of "you", for text that leaves this page. function eventText(ev, named = false) { - const person = ev.user_id != null ? (named ? ev.username || 'someone' : who(ev.user_id, ev.username)) : null; + const person = actorLabel(ev, named); const strong = (t) => h('span', { class: 'who', text: t || 'someone' }); const alertName = () => { const a = (inc.alerts || []).find((x) => x.id === ev.alert_id);