Stop 500ing when a service account acts on an incident
Every incident-mutation handler read userFromContext(ctx) and wrote the result's .ID into acknowledged_by/incident_events.user_id without checking the ok bool. For a team-scoped service-account caller this returned a zero-value user id, which violated the users(id) FK and 500'd on acknowledge, unacknowledge, resolve, snooze, unsnooze and create-note. handleDeleteNote didn't crash but silently matched zero rows instead (WHERE user_id = 0), so a service account could never delete its own note. Add acknowledged_by_service_account_id (incidents) and service_account_id (incident_events) as nullable FKs to service_accounts(id), parallel to and mutually exclusive with the existing human columns (migration 015, with a CHECK enforcing the exclusion). Route every one of the six handlers plus delete-note through a new callerActorIDs() helper that branches on Caller.AsHuman()/ServiceAccountID() instead of assuming a human, and thread a serviceAccountID parameter through logEvent and the new acknowledgeIncidentAs (acknowledgeIncident itself is untouched: its only other caller, the push-notification Acknowledge button, is always human). Render the new actor distinctly from both a human and "the server acted" in the web UI's incident timeline and facts card. handleIncidentAssign, handleIncidentArchive and handleIncidentUnarchive are deliberately not touched here — they track no actor at all today, for anyone, which is a separate pre-existing gap (follow-up issue to come). Fixes #25
This commit is contained in:
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user