From ab61dc362ad7139897216bd6497422dace968364 Mon Sep 17 00:00:00 2001 From: Ameen Rashad <7609895+vazra@users.noreply.github.com> Date: Sun, 11 Oct 2026 02:45:14 -0700 Subject: [PATCH 1/2] fix(api): alert rules keep the app chosen in the UI (accept app_slug) POST/PUT /api/alerts/rules only read app_id while the UI sends app_slug, so app rules became all-apps rules and manage users could not create any. Accept app_slug (resolved and authorized via canMutateForApp), reject unknown slugs and id/slug mismatches. --- docs/guides/alerts/rules.md | 3 +- docs/reference/api.md | 6 +- internal/api/alerts.go | 101 +++++++---- internal/api/alerts_scope_test.go | 233 +++++++++++++++++++++++++ ui/src/routes/__tests__/Alerts.test.js | 77 +++++++- 5 files changed, 372 insertions(+), 48 deletions(-) create mode 100644 internal/api/alerts_scope_test.go diff --git a/docs/guides/alerts/rules.md b/docs/guides/alerts/rules.md index fe0acfa2..d5b33e44 100644 --- a/docs/guides/alerts/rules.md +++ b/docs/guides/alerts/rules.md @@ -17,6 +17,7 @@ curl -X POST https://manage.yourdomain.com/api/alerts/rules \ | Field | Type | Notes | |-------|------|-------| | `app_id` | int | Target app (omit / null for system-wide; super_admin only) | +| `app_slug` | string | Target app by slug, instead of `app_id`. If both are set they must name the same app. An unknown slug is rejected with `400` | | `metric` | string | `cpu_pct`, `mem_bytes`, `mem_pct`, or a backup event (below) | | `operator` | string | `>`, `<`, `>=`, `<=` | | `threshold` | float | Numeric threshold | @@ -35,7 +36,7 @@ Set `metric` to a backup event to get a message when backups run: | `backup_missed` | a scheduled backup is overdue (more than twice its interval) | | `backup_all` | any of the above | -A rule with an `app_id` only reports that app's backups. A system-wide rule reports every app's backups. Backup rules send one message per event, so `threshold` and `duration_sec` are ignored (the API still requires an `operator`; use `>`). +A rule for an app (`app_id` or `app_slug`) only reports that app's backups. A system-wide rule reports every app's backups. Backup rules send one message per event, so `threshold` and `duration_sec` are ignored (the API still requires an `operator`; use `>`). ## Compose label shortcuts diff --git a/docs/reference/api.md b/docs/reference/api.md index e8689d2d..659934bc 100644 --- a/docs/reference/api.md +++ b/docs/reference/api.md @@ -376,12 +376,14 @@ Create alert rule. } ``` -Metrics: `cpu_pct`, `mem_bytes`, `mem_pct`. +Metrics: `cpu_pct`, `mem_bytes`, `mem_pct`, and the backup events `backup_failed`, `backup_success`, `backup_missed`, `backup_all`. Operators: `>`, `<`, `>=`, `<=`. +Name the app with `app_id` or `app_slug` (`"app_slug": "myapp"`). If both are set they must name the same app. Omit both for a system-wide rule (super_admin only). An unknown `app_slug` returns `400` for super_admin; `manage` users get `404` for apps they cannot manage, whether or not the app exists. + ### `PUT /api/alerts/rules/{id}` -Update alert rule. +Update alert rule. Same body as `POST`; send the full rule, since an omitted app makes it system-wide. ### `DELETE /api/alerts/rules/{id}` diff --git a/internal/api/alerts.go b/internal/api/alerts.go index 7a62cc71..e2bfa091 100644 --- a/internal/api/alerts.go +++ b/internal/api/alerts.go @@ -304,16 +304,64 @@ func alertRuleAuditJSON(r *store.AlertRule) []byte { return b } -func (s *Server) handleCreateAlertRule(w http.ResponseWriter, r *http.Request) { - var req struct { - AppID *int64 `json:"app_id"` - Metric string `json:"metric"` - Operator string `json:"operator"` - Threshold float64 `json:"threshold"` - DurationSec int `json:"duration_sec"` - WebhookID int64 `json:"webhook_id"` - Enabled bool `json:"enabled"` +// alertRuleReq is the body of POST and PUT /api/alerts/rules. The app is +// named by app_id or app_slug (the UI sends the slug); neither means a +// global rule. +type alertRuleReq struct { + AppID *int64 `json:"app_id"` + AppSlug string `json:"app_slug"` + Metric string `json:"metric"` + Operator string `json:"operator"` + Threshold float64 `json:"threshold"` + DurationSec int `json:"duration_sec"` + WebhookID int64 `json:"webhook_id"` + Enabled bool `json:"enabled"` +} + +// alertRuleApp resolves the app a rule targets and checks the caller may +// mutate rules for it (canMutateForApp). If both app_id and app_slug are +// set they must name the same app. An unknown slug is a 400 for +// super_admin; other callers get the answer they would get for an app they +// cannot reach, so they cannot probe which apps exist. On failure the +// response is written and ok is false. +func (s *Server) alertRuleApp(w http.ResponseWriter, r *http.Request, req *alertRuleReq) (appID *int64, appSlug string, ok bool) { + if req.AppSlug == "" { + if !s.canMutateForApp(w, r, req.AppID) { + return nil, "", false + } + if req.AppID != nil { + if app, err := s.store.GetAppByID(*req.AppID); err == nil { + appSlug = app.Slug + } + } + return req.AppID, appSlug, true + } + app, err := s.store.GetAppBySlug(req.AppSlug) + if err != nil { + if u := GetAuthUser(r); u != nil && u.Role == "super_admin" { + http.Error(w, fmt.Sprintf("app %q not found", req.AppSlug), http.StatusBadRequest) + return nil, "", false + } + // No app has id 0, so this writes the same 401/403/404 as for an + // existing app the caller has no access to. + var none int64 + if s.canMutateForApp(w, r, &none) { + http.Error(w, "not found", http.StatusNotFound) + } + return nil, "", false + } + if !s.canMutateForApp(w, r, &app.ID) { + return nil, "", false } + if req.AppID != nil && *req.AppID != app.ID { + http.Error(w, "app_id and app_slug name different apps", http.StatusBadRequest) + return nil, "", false + } + return &app.ID, app.Slug, true +} + +func (s *Server) handleCreateAlertRule(w http.ResponseWriter, r *http.Request) { + var req alertRuleReq if err := json.NewDecoder(r.Body).Decode(&req); err != nil { http.Error(w, "invalid JSON", http.StatusBadRequest) return @@ -326,7 +374,8 @@ func (s *Server) handleCreateAlertRule(w http.ResponseWriter, r *http.Request) { http.Error(w, msg, http.StatusBadRequest) return } - if !s.canMutateForApp(w, r, req.AppID) { + appID, appSlug, ok := s.alertRuleApp(w, r, &req) + if !ok { return } // verify webhook exists @@ -335,7 +384,8 @@ func (s *Server) handleCreateAlertRule(w http.ResponseWriter, r *http.Request) { return } rule := &store.AlertRule{ - AppID: req.AppID, + AppID: appID, + AppSlug: appSlug, Metric: req.Metric, Operator: req.Operator, Threshold: req.Threshold, @@ -347,10 +397,6 @@ func (s *Server) handleCreateAlertRule(w http.ResponseWriter, r *http.Request) { httpError(w, err, http.StatusInternalServerError) return } - var appID *int64 - if req.AppID != nil { - appID = req.AppID - } afterJSON := alertRuleAuditJSON(rule) _, _ = s.audit.Record(r.Context(), audit.RecordReq{ Category: "alert", @@ -400,15 +446,7 @@ func (s *Server) handleUpdateAlertRule(w http.ResponseWriter, r *http.Request) { } } } - var req struct { - AppID *int64 `json:"app_id"` - Metric string `json:"metric"` - Operator string `json:"operator"` - Threshold float64 `json:"threshold"` - DurationSec int `json:"duration_sec"` - WebhookID int64 `json:"webhook_id"` - Enabled bool `json:"enabled"` - } + var req alertRuleReq if err := json.NewDecoder(r.Body).Decode(&req); err != nil { http.Error(w, "invalid JSON", http.StatusBadRequest) return @@ -422,18 +460,13 @@ func (s *Server) handleUpdateAlertRule(w http.ResponseWriter, r *http.Request) { if existing != nil && !s.canMutateForApp(w, r, existing.AppID) { return } - if !s.canMutateForApp(w, r, req.AppID) { + appID, appSlug, ok := s.alertRuleApp(w, r, &req) + if !ok { return } - var appSlug string - if req.AppID != nil { - if app, err := s.store.GetAppByID(*req.AppID); err == nil { - appSlug = app.Slug - } - } rule := &store.AlertRule{ ID: id, - AppID: req.AppID, + AppID: appID, AppSlug: appSlug, Metric: req.Metric, Operator: req.Operator, @@ -451,10 +484,6 @@ func (s *Server) handleUpdateAlertRule(w http.ResponseWriter, r *http.Request) { beforeJSON = alertRuleAuditJSON(existing) } afterJSON := alertRuleAuditJSON(rule) - var appID *int64 - if req.AppID != nil { - appID = req.AppID - } _, _ = s.audit.Record(r.Context(), audit.RecordReq{ Category: "alert", Action: "changed", diff --git a/internal/api/alerts_scope_test.go b/internal/api/alerts_scope_test.go new file mode 100644 index 00000000..40a59c13 --- /dev/null +++ b/internal/api/alerts_scope_test.go @@ -0,0 +1,233 @@ +package api + +import ( + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "testing" + + "github.com/vazra/simpledeploy/internal/store" +) + +// Alert rule app scope by slug: the UI sends app_slug, not app_id. + +// seedAlertApps creates apps "alpha" and "beta" and returns their ids. +func seedAlertApps(t *testing.T, st *store.Store) (alphaID, betaID int64) { + t.Helper() + for _, slug := range []string{"alpha", "beta"} { + if err := st.UpsertApp(&store.App{Name: slug, Slug: slug, ComposePath: "/tmp/" + slug + ".yml", Status: "running"}, nil); err != nil { + t.Fatalf("seed %s: %v", slug, err) + } + } + a, _ := st.GetAppBySlug("alpha") + b, _ := st.GetAppBySlug("beta") + return a.ID, b.ID +} + +func doRuleRequest(t *testing.T, srv *Server, cookie *http.Cookie, method, path string, body map[string]any) (*httptest.ResponseRecorder, map[string]any) { + t.Helper() + w := httptest.NewRecorder() + srv.Handler().ServeHTTP(w, authedRequest(t, method, path, body, cookie)) + var resp map[string]any + _ = json.Unmarshal(w.Body.Bytes(), &resp) + return w, resp +} + +func ruleBody(whID int64, threshold float64, scope map[string]any) map[string]any { + b := map[string]any{ + "metric": "cpu_pct", "operator": ">", "threshold": threshold, + "duration_sec": 60, "webhook_id": whID, "enabled": true, + } + for k, v := range scope { + b[k] = v + } + return b +} + +func storedRule(t *testing.T, st *store.Store, id int64) store.AlertRule { + t.Helper() + rules, err := st.ListAlertRules(nil) + if err != nil { + t.Fatalf("list rules: %v", err) + } + for _, r := range rules { + if r.ID == id { + return r + } + } + t.Fatalf("rule %d not stored", id) + return store.AlertRule{} +} + +func mustUserID(t *testing.T, st *store.Store, username string) int64 { + t.Helper() + u, err := st.GetUserByUsername(username) + if err != nil { + t.Fatalf("get user %s: %v", username, err) + } + return u.ID +} + +func TestAlertRuleAppSlugKeepsApp(t *testing.T) { + srv, st, cookie := setupAlertTestServer(t) + alphaID, _ := seedAlertApps(t, st) + whID := int64(createTestWebhook(t, srv, cookie, "slugwh")["id"].(float64)) + + w, resp := doRuleRequest(t, srv, cookie, http.MethodPost, "/api/alerts/rules", ruleBody(whID, 80, map[string]any{"app_slug": "alpha"})) + if w.Code != http.StatusCreated { + t.Fatalf("create status = %d, want 201; body: %s", w.Code, w.Body.String()) + } + if resp["app_slug"] != "alpha" { + t.Errorf("create response app_slug = %v, want alpha", resp["app_slug"]) + } + ruleID := int64(resp["id"].(float64)) + if got := storedRule(t, st, ruleID); got.AppID == nil || *got.AppID != alphaID { + t.Fatalf("created rule app_id = %v, want %d (rule became all-apps)", got.AppID, alphaID) + } + + // Editing in the UI sends app_slug again; the app must stay. + path := fmt.Sprintf("/api/alerts/rules/%d", ruleID) + w, resp = doRuleRequest(t, srv, cookie, http.MethodPut, path, ruleBody(whID, 90, map[string]any{"app_slug": "alpha"})) + if w.Code != http.StatusOK { + t.Fatalf("update status = %d, want 200; body: %s", w.Code, w.Body.String()) + } + if resp["app_slug"] != "alpha" { + t.Errorf("update response app_slug = %v, want alpha", resp["app_slug"]) + } + got := storedRule(t, st, ruleID) + if got.AppID == nil || *got.AppID != alphaID { + t.Fatalf("updated rule app_id = %v, want %d (edit turned rule into all-apps)", got.AppID, alphaID) + } + if got.Threshold != 90 { + t.Errorf("threshold = %v, want 90", got.Threshold) + } + + // app_id plus a matching app_slug (a GET round trip) is accepted. + w, _ = doRuleRequest(t, srv, cookie, http.MethodPut, path, ruleBody(whID, 70, map[string]any{"app_id": alphaID, "app_slug": "alpha"})) + if w.Code != http.StatusOK { + t.Fatalf("update with matching app_id+app_slug status = %d; body: %s", w.Code, w.Body.String()) + } + if got := storedRule(t, st, ruleID); got.AppID == nil || *got.AppID != alphaID { + t.Fatalf("rule app_id = %v, want %d", got.AppID, alphaID) + } + + // Empty app_slug (the "All Apps" option) makes it global for super_admin. + w, _ = doRuleRequest(t, srv, cookie, http.MethodPut, path, ruleBody(whID, 70, map[string]any{"app_slug": ""})) + if w.Code != http.StatusOK { + t.Fatalf("update to all apps status = %d; body: %s", w.Code, w.Body.String()) + } + if got := storedRule(t, st, ruleID); got.AppID != nil { + t.Fatalf("rule app_id = %v, want nil (all apps)", *got.AppID) + } +} + +func TestAlertRuleAppSlugValidation(t *testing.T) { + srv, st, cookie := setupAlertTestServer(t) + alphaID, _ := seedAlertApps(t, st) + whID := int64(createTestWebhook(t, srv, cookie, "valslugwh")["id"].(float64)) + ruleID := int64(createTestRule(t, srv, cookie, whID)["id"].(float64)) + path := fmt.Sprintf("/api/alerts/rules/%d", ruleID) + + for _, tc := range []struct { + name string + method string + path string + scope map[string]any + }{ + {"create unknown slug", http.MethodPost, "/api/alerts/rules", map[string]any{"app_slug": "nope"}}, + {"update unknown slug", http.MethodPut, path, map[string]any{"app_slug": "nope"}}, + {"create mismatched id and slug", http.MethodPost, "/api/alerts/rules", map[string]any{"app_id": alphaID, "app_slug": "beta"}}, + {"update mismatched id and slug", http.MethodPut, path, map[string]any{"app_id": alphaID, "app_slug": "beta"}}, + } { + w, _ := doRuleRequest(t, srv, cookie, tc.method, tc.path, ruleBody(whID, 50, tc.scope)) + if w.Code != http.StatusBadRequest { + t.Errorf("%s: status = %d, want 400; body: %s", tc.name, w.Code, w.Body.String()) + } + } + if got := storedRule(t, st, ruleID); got.AppID != nil || got.Threshold != 80 { + t.Errorf("rejected updates changed the rule: %+v", got) + } + if rules, _ := st.ListAlertRules(nil); len(rules) != 1 { + t.Errorf("rejected creates stored rules: have %d rules, want 1", len(rules)) + } +} + +func TestAlertRuleAppSlugManageUserScope(t *testing.T) { + srv, st, adminCookie := setupUserTestServer(t) + alphaID, betaID := seedAlertApps(t, st) + whID := int64(createTestWebhook(t, srv, adminCookie, "mgrwh")["id"].(float64)) + + mgrCookie := loginAs(t, srv, st, "mgr", "managepass1", "manage") + if err := st.GrantAppAccess(mustUserID(t, st, "mgr"), alphaID); err != nil { + t.Fatalf("grant: %v", err) + } + + // Own app by slug: allowed and kept. + w, resp := doRuleRequest(t, srv, mgrCookie, http.MethodPost, "/api/alerts/rules", ruleBody(whID, 80, map[string]any{"app_slug": "alpha"})) + if w.Code != http.StatusCreated { + t.Fatalf("manage create for own app status = %d, want 201; body: %s", w.Code, w.Body.String()) + } + ownID := int64(resp["id"].(float64)) + if got := storedRule(t, st, ownID); got.AppID == nil || *got.AppID != alphaID { + t.Fatalf("manage rule app_id = %v, want %d", got.AppID, alphaID) + } + + // Another app, or an app that does not exist: same 404, nothing stored. + for _, slug := range []string{"beta", "nope"} { + w, _ = doRuleRequest(t, srv, mgrCookie, http.MethodPost, "/api/alerts/rules", ruleBody(whID, 80, map[string]any{"app_slug": slug})) + if w.Code != http.StatusNotFound { + t.Errorf("manage create for %q status = %d, want 404; body: %s", slug, w.Code, w.Body.String()) + } + } + if rules, _ := st.ListAlertRules(nil); len(rules) != 1 { + t.Fatalf("manage stored %d rules, want 1", len(rules)) + } + + // Moving their own rule to another app is refused and leaves it alone. + ownPath := fmt.Sprintf("/api/alerts/rules/%d", ownID) + w, _ = doRuleRequest(t, srv, mgrCookie, http.MethodPut, ownPath, ruleBody(whID, 99, map[string]any{"app_slug": "beta"})) + if w.Code != http.StatusNotFound { + t.Errorf("manage move to beta status = %d, want 404", w.Code) + } + // Mismatched id and slug, where the slug is an app they lack: 404, not + // 400, so the answer does not reveal that the app exists. + w, _ = doRuleRequest(t, srv, mgrCookie, http.MethodPut, ownPath, ruleBody(whID, 99, map[string]any{"app_id": alphaID, "app_slug": "beta"})) + if w.Code != http.StatusNotFound { + t.Errorf("manage mismatched id/slug status = %d, want 404", w.Code) + } + // Global rules stay super_admin only. + w, _ = doRuleRequest(t, srv, mgrCookie, http.MethodPut, ownPath, ruleBody(whID, 99, map[string]any{"app_slug": ""})) + if w.Code != http.StatusForbidden { + t.Errorf("manage make global status = %d, want 403", w.Code) + } + if got := storedRule(t, st, ownID); got.AppID == nil || *got.AppID != alphaID || got.Threshold != 80 { + t.Errorf("refused updates changed the rule: %+v", got) + } + + // A rule on an app they lack cannot be pulled into their app. + w, resp = doRuleRequest(t, srv, adminCookie, http.MethodPost, "/api/alerts/rules", ruleBody(whID, 80, map[string]any{"app_slug": "beta"})) + if w.Code != http.StatusCreated { + t.Fatalf("admin create beta rule status = %d; body: %s", w.Code, w.Body.String()) + } + betaRuleID := int64(resp["id"].(float64)) + w, _ = doRuleRequest(t, srv, mgrCookie, http.MethodPut, fmt.Sprintf("/api/alerts/rules/%d", betaRuleID), ruleBody(whID, 1, map[string]any{"app_slug": "alpha"})) + if w.Code != http.StatusNotFound { + t.Errorf("manage edit of beta rule status = %d, want 404", w.Code) + } + if got := storedRule(t, st, betaRuleID); got.AppID == nil || *got.AppID != betaID || got.Threshold != 80 { + t.Errorf("beta rule changed: %+v", got) + } + + // Viewers cannot create rules, for their own app or a made-up one. + viewerCookie := loginAs(t, srv, st, "v1", "viewerpass1", "viewer") + if err := st.GrantAppAccess(mustUserID(t, st, "v1"), alphaID); err != nil { + t.Fatalf("grant viewer: %v", err) + } + for _, slug := range []string{"alpha", "nope"} { + w, _ = doRuleRequest(t, srv, viewerCookie, http.MethodPost, "/api/alerts/rules", ruleBody(whID, 80, map[string]any{"app_slug": slug})) + if w.Code != http.StatusForbidden { + t.Errorf("viewer create for %q status = %d, want 403", slug, w.Code) + } + } +} diff --git a/ui/src/routes/__tests__/Alerts.test.js b/ui/src/routes/__tests__/Alerts.test.js index 05e6e033..68a7ae95 100644 --- a/ui/src/routes/__tests__/Alerts.test.js +++ b/ui/src/routes/__tests__/Alerts.test.js @@ -1,15 +1,16 @@ -import { describe, it, expect, vi } from 'vitest'; -import { render, waitFor } from '@testing-library/svelte'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { render, waitFor, fireEvent, screen } from '@testing-library/svelte'; + +const apiMock = vi.hoisted(() => ({})); vi.mock('../../lib/api.js', async () => { const { makeApiMock } = await import('../../test-mocks/api.js'); - return { - api: makeApiMock({ - listWebhooks: vi.fn(async () => ({ data: [], error: null })), - listAlertRules: vi.fn(async () => ({ data: [], error: null })), - alertHistory: vi.fn(async () => ({ data: [], error: null })), - }), - }; + Object.assign(apiMock, makeApiMock({ + listWebhooks: vi.fn(async () => ({ data: [], error: null })), + listAlertRules: vi.fn(async () => ({ data: [], error: null })), + alertHistory: vi.fn(async () => ({ data: [], error: null })), + })); + return { api: apiMock }; }); vi.mock('svelte-spa-router', () => ({ push: vi.fn(), link: (n) => n, default: () => null })); @@ -17,10 +18,68 @@ vi.mock('../../components/Layout.svelte', async () => await import('./LayoutStub import Alerts from '../Alerts.svelte'; +const ok = (data) => ({ data, error: null }); + +function selectWithOption(text) { + return screen.getByRole('option', { name: text }).closest('select'); +} + +async function choose(select, value) { + select.value = value; + await fireEvent.change(select); +} + describe('Alerts route', () => { it('renders without crashing and mentions Alerts/webhooks', async () => { const { container } = render(Alerts); await waitFor(() => expect(container.textContent.length).toBeGreaterThan(0)); expect(container.textContent.toLowerCase()).toMatch(/alert|webhook|rule/); }); + + describe('rule form payload', () => { + const rule = { + id: 9, app_id: 1, app_slug: 'alpha', metric: 'cpu_pct', operator: '>', + threshold: 80, duration_sec: 60, webhook_id: 5, enabled: true, + }; + + beforeEach(() => { + apiMock.listApps.mockResolvedValue(ok([{ ID: 1, Slug: 'alpha' }, { ID: 2, Slug: 'beta' }])); + apiMock.listWebhooks.mockResolvedValue(ok([{ id: 5, name: 'Ops', type: 'slack' }])); + apiMock.listAlertRules.mockResolvedValue(ok([rule])); + apiMock.createAlertRule.mockClear(); + apiMock.updateAlertRule.mockClear(); + }); + + it('sends the chosen app when creating a rule', async () => { + render(Alerts); + await fireEvent.click(await screen.findByText('Add Rule')); + await choose(selectWithOption('All Apps'), 'beta'); + await choose(selectWithOption('Select webhook'), '5'); + await fireEvent.click(screen.getByRole('button', { name: 'Create' })); + await waitFor(() => expect(apiMock.createAlertRule).toHaveBeenCalledTimes(1)); + expect(apiMock.createAlertRule.mock.calls[0][0]).toMatchObject({ + app_slug: 'beta', metric: 'cpu_pct', webhook_id: 5, enabled: true, + }); + }); + + it('sends an empty app for an all-apps rule', async () => { + render(Alerts); + await fireEvent.click(await screen.findByText('Add Rule')); + await choose(selectWithOption('Select webhook'), '5'); + await fireEvent.click(screen.getByRole('button', { name: 'Create' })); + await waitFor(() => expect(apiMock.createAlertRule).toHaveBeenCalledTimes(1)); + expect(apiMock.createAlertRule.mock.calls[0][0].app_slug).toBe(''); + }); + + it('keeps the rule app when editing a rule', async () => { + render(Alerts); + await fireEvent.click(await screen.findByText(/CPU Usage > 80\.0% - alpha/)); + expect(selectWithOption('All Apps').value).toBe('alpha'); + await fireEvent.click(screen.getByRole('button', { name: 'Save' })); + await waitFor(() => expect(apiMock.updateAlertRule).toHaveBeenCalledTimes(1)); + const [id, payload] = apiMock.updateAlertRule.mock.calls[0]; + expect(id).toBe(9); + expect(payload).toMatchObject({ app_slug: 'alpha', webhook_id: 5 }); + }); + }); }); From 49089ff402989968022354877790ac05629a260e Mon Sep 17 00:00:00 2001 From: Ameen Rashad <7609895+vazra@users.noreply.github.com> Date: Sun, 11 Oct 2026 10:02:11 -0700 Subject: [PATCH 2/2] fix: refresh backup views live when backup and restore runs start and finish --- cmd/simpledeploy/main.go | 1 + docs/architecture/realtime.md | 4 +- internal/api/events_ws.go | 7 +- internal/api/events_ws_test.go | 43 +++++ internal/backup/scheduler.go | 57 ++++++- internal/backup/scheduler_events_test.go | 153 ++++++++++++++++++ internal/events/bus.go | 4 + internal/events/topics.go | 15 ++ ui/src/components/BackupsTab.svelte | 12 +- .../components/__tests__/BackupsTab.test.js | 18 +++ 10 files changed, 301 insertions(+), 13 deletions(-) create mode 100644 internal/backup/scheduler_events_test.go diff --git a/cmd/simpledeploy/main.go b/cmd/simpledeploy/main.go index 57fb9113..88bbd1ed 100644 --- a/cmd/simpledeploy/main.go +++ b/cmd/simpledeploy/main.go @@ -731,6 +731,7 @@ func runServe(cmd *cobra.Command, args []string) error { json.Unmarshal([]byte(decrypted), &s3cfg) return backup.NewS3Target(s3cfg) }) + backupSched.SetEventBus(eventBus) backupSched.SetAlertFunc(func(appID int64, appName, strategy, message, eventType string) { evaluator.DispatchBackupAlert(alerts.BackupAlertEvent{ AppID: appID, diff --git a/docs/architecture/realtime.md b/docs/architecture/realtime.md index 101e6dce..b5687dfc 100644 --- a/docs/architecture/realtime.md +++ b/docs/architecture/realtime.md @@ -35,7 +35,7 @@ SimpleDeploy keeps the dashboard fresh without polling by broadcasting tiny noti | `global:users` | users, role changes, access grants/revokes, API keys | | `global:registries`| registry CRUD | | `global:alerts` | webhooks, alert rules | -| `global:backups` | any backup config or run | +| `global:backups` | any backup config change; backup and restore runs starting and finishing | | `global:docker` | docker prune actions | | `global:audit` | every audit-recorded mutation | @@ -62,7 +62,7 @@ No payload data ever ships in events. The UI runs its existing REST refetch when ## Authorization -Topic ACL is computed when the WS opens. `super_admin` sees every global topic; everyone else sees `global:apps`, `global:backups`, `global:alerts` plus `app:` for any slug they have `user_app_access` on. A `sub` for a forbidden topic returns `{op:"err", reason:"forbidden"}`. When an `access` or `user` audit event affects the connected user, the server closes the socket so the client reconnects with fresh authz. +Topic ACL is computed when the WS opens. `super_admin` sees every global topic; everyone else sees `global:apps`, `global:backups`, `global:alerts` plus `app:` for any slug they have `user_app_access` on. A `sub` for a forbidden topic returns `{op:"err", reason:"forbidden"}`. An event about one app (`Event.App` set, never sent to clients) is delivered only to users who can view that app, on any topic: the backup scheduler publishes `backup.run` on both `app:` and `global:backups` when a run starts and finishes, and a user with access to other apps does not learn that this app's backups ran. When an `access` or `user` audit event affects the connected user, the server closes the socket so the client reconnects with fresh authz. The upgrade requires a matching `Origin` (host and port) or `Sec-Fetch-Site: same-origin`, like the log sockets (`checkWebSocketOrigin`, see [Auth](/architecture/auth/#cross-origin-protection)). Every 60s the socket re-runs `wsAuthStillValid`: if the user is gone, their role changed, or the session token or API key used to connect is no longer valid, it closes with code 1008. `app:` topics the user lost access to are dropped with an `err` frame. The app log, deploy log and process log sockets run the same 60s check and close on failure. diff --git a/internal/api/events_ws.go b/internal/api/events_ws.go index 47c5d455..a7cad9c6 100644 --- a/internal/api/events_ws.go +++ b/internal/api/events_ws.go @@ -142,10 +142,13 @@ func (s *Server) handleEventsWS(w http.ResponseWriter, r *http.Request) { uid := user.ID // Filter: deliver only events whose topic this conn is subscribed to. + // Events about one app (App set) go only to users who can view it, on + // any topic, global ones included. filter := func(e events.Event) bool { mu.Lock() - defer mu.Unlock() - return subs[e.Topic] + subscribed := subs[e.Topic] + mu.Unlock() + return subscribed && (e.App == "" || s.canSubscribeApp(user, e.App)) } ch, cancel, sub := s.bus.Subscribe(filter) defer cancel() diff --git a/internal/api/events_ws_test.go b/internal/api/events_ws_test.go index d7515207..baf67ca4 100644 --- a/internal/api/events_ws_test.go +++ b/internal/api/events_ws_test.go @@ -12,6 +12,7 @@ import ( "github.com/gorilla/websocket" "github.com/vazra/simpledeploy/internal/events" + "github.com/vazra/simpledeploy/internal/store" ) func newEventsTestServer(t *testing.T) (*httptest.Server, *Server, *http.Cookie) { @@ -134,3 +135,45 @@ func TestEventsWSUnsub(t *testing.T) { t.Fatal("expected no frame after unsub") } } + +// Events tagged with an app reach only users who can view that app, even on +// a global topic every user may subscribe to. +func TestEventsWSAppTaggedEventsFiltered(t *testing.T) { + srv, st, _ := newAuditTestServer(t) + srv.SetBus(events.New()) + ts := httptest.NewServer(srv.Handler()) + t.Cleanup(ts.Close) + for _, slug := range []string{"mine", "theirs"} { + if err := st.UpsertApp(&store.App{Name: slug, Slug: slug, ComposePath: "/x/" + slug + ".yml", Status: "running"}, nil); err != nil { + t.Fatal(err) + } + } + mine, err := st.GetAppBySlug("mine") + if err != nil { + t.Fatal(err) + } + conn := dialEvents(t, ts, makeUserCookie(t, srv, st, "mgr", mine.ID)) + if err := conn.WriteJSON(map[string]string{"op": "sub", "topic": events.TopicGlobalBackups}); err != nil { + t.Fatal(err) + } + time.Sleep(50 * time.Millisecond) + + for _, e := range events.BackupRunEvents("theirs") { + srv.bus.Publish(t.Context(), e) + } + for _, e := range events.BackupRunEvents("mine") { + srv.bus.Publish(t.Context(), e) + } + f := readFrame(t, conn, 2*time.Second) + if f["topic"] != events.TopicGlobalBackups || f["type"] != events.TypeBackupRun { + t.Fatalf("first frame = %v, want the event for the user's own app", f) + } + if _, leaked := f["app"]; leaked { + t.Fatalf("frame exposes the app slug: %v", f) + } + // Nothing else: the other app's event was dropped. + conn.SetReadDeadline(time.Now().Add(300 * time.Millisecond)) + if _, data, err := conn.ReadMessage(); err == nil { + t.Fatalf("unexpected extra frame: %s", data) + } +} diff --git a/internal/backup/scheduler.go b/internal/backup/scheduler.go index e20e67f4..b6346dd9 100644 --- a/internal/backup/scheduler.go +++ b/internal/backup/scheduler.go @@ -13,12 +13,19 @@ import ( "github.com/robfig/cron/v3" "github.com/vazra/simpledeploy/internal/compose" + "github.com/vazra/simpledeploy/internal/events" "github.com/vazra/simpledeploy/internal/store" ) // BackupAlertFunc is a callback for backup-related alerts. type BackupAlertFunc func(appID int64, appName, strategy, message, eventType string) +// EventPublisher receives realtime notify-only events. *events.Bus +// satisfies it. +type EventPublisher interface { + Publish(ctx context.Context, e events.Event) +} + // BackupStore is the subset of store.Store needed by Scheduler. type BackupStore interface { ListBackupConfigs(appID *int64) ([]store.BackupConfig, error) @@ -42,6 +49,7 @@ type Scheduler struct { entries map[int64]cron.EntryID hookExec ContainerExecutor alertFunc BackupAlertFunc + bus EventPublisher mu sync.Mutex } @@ -72,6 +80,36 @@ func (s *Scheduler) SetAlertFunc(fn BackupAlertFunc) { s.alertFunc = fn } +// SetEventBus wires the realtime events bus, told when a backup or restore +// run starts and finishes so open Backups views refresh. nil disables it. +func (s *Scheduler) SetEventBus(b EventPublisher) { + s.bus = b +} + +// publishRun notifies the UI that a run of app slug started or finished. +func (s *Scheduler) publishRun(ctx context.Context, slug string) { + if s.bus == nil || slug == "" { + return + } + for _, e := range events.BackupRunEvents(slug) { + s.bus.Publish(ctx, e) + } +} + +// appSlug returns the slug of app appID, or "" if it cannot be loaded. +func (s *Scheduler) appSlug(appID int64) string { + if app, err := s.store.GetAppByID(appID); err == nil && app != nil { + return app.Slug + } + return "" +} + +// failRun marks run failed and notifies the UI. +func (s *Scheduler) failRun(ctx context.Context, runID int64, slug, errMsg string) { + _ = s.store.UpdateBackupRunFailed(runID, errMsg) + s.publishRun(ctx, slug) +} + // GetStrategy returns a registered strategy by name. func (s *Scheduler) GetStrategy(name string) (Strategy, bool) { st, ok := s.strategies[name] @@ -160,39 +198,41 @@ func (s *Scheduler) RunBackup(ctx context.Context, cfgID int64) error { if err != nil { return fmt.Errorf("create backup run: %w", err) } + slug := s.appSlug(cfg.AppID) + s.publishRun(ctx, slug) strategy, ok := s.strategies[cfg.Strategy] if !ok { errMsg := fmt.Sprintf("unknown strategy: %s", cfg.Strategy) - _ = s.store.UpdateBackupRunFailed(run.ID, errMsg) + s.failRun(ctx, run.ID, slug, errMsg) return fmt.Errorf("%s", errMsg) } factory, ok := s.targets[cfg.Target] if !ok { errMsg := fmt.Sprintf("unknown target: %s", cfg.Target) - _ = s.store.UpdateBackupRunFailed(run.ID, errMsg) + s.failRun(ctx, run.ID, slug, errMsg) return fmt.Errorf("%s", errMsg) } target, err := factory(cfg.TargetConfigJSON) if err != nil { errMsg := fmt.Sprintf("create target: %v", err) - _ = s.store.UpdateBackupRunFailed(run.ID, errMsg) + s.failRun(ctx, run.ID, slug, errMsg) return fmt.Errorf("%s", errMsg) } app, err := s.store.GetAppByID(cfg.AppID) if err != nil { errMsg := fmt.Sprintf("get app: %v", err) - _ = s.store.UpdateBackupRunFailed(run.ID, errMsg) + s.failRun(ctx, run.ID, slug, errMsg) return fmt.Errorf("%s", errMsg) } plan, err := s.planRun(cfg, app, strategy, target) if err != nil { errMsg := err.Error() - _ = s.store.UpdateBackupRunFailed(run.ID, errMsg) + s.failRun(ctx, run.ID, slug, errMsg) s.sendAlert(app.ID, app.Name, cfg.Strategy, fmt.Sprintf("backup failed: %s", errMsg), "backup_failed") return fmt.Errorf("%s", errMsg) } @@ -205,17 +245,19 @@ func (s *Scheduler) RunBackup(ctx context.Context, cfgID int64) error { result, err := plan.pipe.RunBackup(ctx, opts, plan.preHooks, plan.postHooks) if err != nil { errMsg := err.Error() - _ = s.store.UpdateBackupRunFailed(run.ID, errMsg) + s.failRun(ctx, run.ID, slug, errMsg) s.sendAlert(app.ID, app.Name, cfg.Strategy, fmt.Sprintf("backup failed: %s", errMsg), "backup_failed") return fmt.Errorf("pipeline: %w", err) } if err := s.store.UpdateBackupRunSuccess(run.ID, result.SizeBytes, result.FilePath, result.Checksum); err != nil { + s.publishRun(ctx, slug) return fmt.Errorf("update run success: %w", err) } // Prune old runs based on retention mode s.pruneOldRuns(ctx, cfg, target) + s.publishRun(ctx, slug) s.sendAlert(app.ID, app.Name, cfg.Strategy, "backup completed successfully", "backup_success") return nil @@ -232,6 +274,9 @@ func (s *Scheduler) RunRestore(ctx context.Context, runID int64) error { if err != nil { return fmt.Errorf("get backup config: %w", err) } + slug := s.appSlug(cfg.AppID) + s.publishRun(ctx, slug) + defer s.publishRun(ctx, slug) strategy, ok := s.strategies[cfg.Strategy] if !ok { diff --git a/internal/backup/scheduler_events_test.go b/internal/backup/scheduler_events_test.go new file mode 100644 index 00000000..43cb45e0 --- /dev/null +++ b/internal/backup/scheduler_events_test.go @@ -0,0 +1,153 @@ +package backup + +import ( + "context" + "errors" + "fmt" + "sync" + "testing" + + "github.com/vazra/simpledeploy/internal/events" + "github.com/vazra/simpledeploy/internal/store" +) + +// recordingPublisher records published events with a note taken at publish +// time (e.g. the run's status then). +type recordingPublisher struct { + mu sync.Mutex + note func() string + events []string +} + +func (p *recordingPublisher) Publish(_ context.Context, e events.Event) { + p.mu.Lock() + defer p.mu.Unlock() + n := "" + if p.note != nil { + n = p.note() + } + p.events = append(p.events, fmt.Sprintf("%s %s app=%s %s", e.Type, e.Topic, e.App, n)) +} + +func (p *recordingPublisher) got() []string { + p.mu.Lock() + defer p.mu.Unlock() + return append([]string(nil), p.events...) +} + +func assertEvents(t *testing.T, got, want []string) { + t.Helper() + if len(got) != len(want) { + t.Fatalf("events = %q, want %q", got, want) + } + for i := range want { + if got[i] != want[i] { + t.Fatalf("events = %q, want %q", got, want) + } + } +} + +func TestRunBackupPublishesStartAndFinish(t *testing.T) { + for _, tc := range []struct { + name string + strategy Strategy + final string + }{ + {"success", &mockStrategy{data: "d", filename: "b.tar.gz"}, "success"}, + {"failure", &mockStrategy{err: errors.New("boom")}, "failed"}, + } { + t.Run(tc.name, func(t *testing.T) { + st := newMockStore() + st.configs[1] = &store.BackupConfig{ID: 1, AppID: 10, Strategy: "mock", Target: "mock", RetentionMode: "count", RetentionCount: 5} + sched := NewScheduler(st, nil) + sched.RegisterStrategy("mock", tc.strategy) + sched.RegisterTargetFactory("mock", func(string) (Target, error) { return newMockTarget(), nil }) + pub := &recordingPublisher{note: func() string { + if r, ok := st.runs[1]; ok { + return r.Status + } + return "no-run" + }} + sched.SetEventBus(pub) + var alertEvents int + sched.SetAlertFunc(func(int64, string, string, string, string) { alertEvents = len(pub.got()) }) + + _ = sched.RunBackup(context.Background(), 1) + + // Started once the run row exists, finished once its status is final. + assertEvents(t, pub.got(), []string{ + "backup.run app:testapp app=testapp running", + "backup.run global:backups app=testapp running", + "backup.run app:testapp app=testapp " + tc.final, + "backup.run global:backups app=testapp " + tc.final, + }) + // The UI hears about the result before slow webhook alerts go out. + if alertEvents != 4 { + t.Errorf("alert sent after %d events, want 4", alertEvents) + } + }) + } +} + +func TestRunBackupPublishesWhenRunFailsEarly(t *testing.T) { + st := newMockStore() + st.configs[1] = &store.BackupConfig{ID: 1, AppID: 10, Strategy: "unknown", Target: "mock"} + sched := NewScheduler(st, nil) + pub := &recordingPublisher{} + sched.SetEventBus(pub) + + if err := sched.RunBackup(context.Background(), 1); err == nil { + t.Fatal("expected error for unknown strategy") + } + if st.failedCall == nil { + t.Fatal("run not marked failed") + } + if got := pub.got(); len(got) != 4 { + t.Fatalf("events = %q, want started and finished on both topics", got) + } +} + +func TestRunRestorePublishesStartAndFinish(t *testing.T) { + for _, tc := range []struct { + name string + err error + }{ + {"success", nil}, + {"failure", errors.New("boom")}, + } { + t.Run(tc.name, func(t *testing.T) { + st := newMockStore() + st.configs[1] = &store.BackupConfig{ID: 1, AppID: 10, Strategy: "mock", Target: "mock"} + st.runs[7] = &store.BackupRun{ID: 7, BackupConfigID: 1, Status: "success", FilePath: "b.tar.gz"} + tgt := newMockTarget() + tgt.uploaded["b.tar.gz"] = []byte("payload") + sched := NewScheduler(st, nil) + sched.RegisterStrategy("mock", &mockStrategy{err: tc.err}) + sched.RegisterTargetFactory("mock", func(string) (Target, error) { return tgt, nil }) + pub := &recordingPublisher{} + sched.SetEventBus(pub) + + err := sched.RunRestore(context.Background(), 7) + if (err != nil) != (tc.err != nil) { + t.Fatalf("RunRestore err = %v, want error %v", err, tc.err != nil) + } + assertEvents(t, pub.got(), []string{ + "backup.run app:testapp app=testapp ", + "backup.run global:backups app=testapp ", + "backup.run app:testapp app=testapp ", + "backup.run global:backups app=testapp ", + }) + }) + } +} + +func TestSchedulerWithoutEventBus(t *testing.T) { + st := newMockStore() + st.configs[1] = &store.BackupConfig{ID: 1, AppID: 10, Strategy: "mock", Target: "mock"} + sched := NewScheduler(st, nil) + sched.RegisterStrategy("mock", &mockStrategy{data: "d", filename: "b.tar.gz"}) + sched.RegisterTargetFactory("mock", func(string) (Target, error) { return newMockTarget(), nil }) + if err := sched.RunBackup(context.Background(), 1); err != nil { + t.Fatalf("RunBackup without bus: %v", err) + } +} diff --git a/internal/events/bus.go b/internal/events/bus.go index 4ff70f2b..f150280b 100644 --- a/internal/events/bus.go +++ b/internal/events/bus.go @@ -14,10 +14,14 @@ import ( // Type is a coarse classification (e.g. "app.changed", "app.status"). // Topic identifies the resource scope (e.g. "app:foo", "global:settings"). // No payload data ships in events; clients refetch the relevant REST resource. +// App, when set, is the slug of the app the event is about. It is never sent +// to clients; the WS handler delivers the event (on any topic, global ones +// included) only to users who can view that app. type Event struct { Type string `json:"type"` Topic string `json:"topic"` ActorID *int64 `json:"actor_id,omitempty"` + App string `json:"-"` Ts time.Time `json:"ts"` } diff --git a/internal/events/topics.go b/internal/events/topics.go index 4ffbb1e4..d2395b79 100644 --- a/internal/events/topics.go +++ b/internal/events/topics.go @@ -20,6 +20,21 @@ const ( // AppTopic returns the per-app topic string for a slug. func AppTopic(slug string) string { return fmt.Sprintf("app:%s", slug) } +// TypeBackupRun is the event type sent when a backup or restore run of an +// app starts or finishes. +const TypeBackupRun = "backup.run" + +// BackupRunEvents returns the events to publish when a backup or restore run +// of app slug starts or finishes: one on the app topic (the app's Backups +// tab) and one on global:backups (the Backups page). Both carry App so only +// users who can view the app receive them. +func BackupRunEvents(slug string) []Event { + return []Event{ + {Type: TypeBackupRun, Topic: AppTopic(slug), App: slug}, + {Type: TypeBackupRun, Topic: TopicGlobalBackups, App: slug}, + } +} + // TopicsForAudit returns the list of topics that should fire for a given // audit category, action, and (optional) app slug. Used by the audit recorder // to translate mutation events into bus publishes. diff --git a/ui/src/components/BackupsTab.svelte b/ui/src/components/BackupsTab.svelte index 792a4518..79284c37 100644 --- a/ui/src/components/BackupsTab.svelte +++ b/ui/src/components/BackupsTab.svelte @@ -7,6 +7,7 @@ import Skeleton from './Skeleton.svelte' import BackupWizard from './BackupWizard.svelte' import { toasts } from '../lib/stores/toast.js' + import { realtime } from '../lib/stores/realtime.svelte.js' let { slug, allowCommandHooks = false } = $props() @@ -29,10 +30,15 @@ let lastRun = $derived(runs[0]) - onMount(loadData) + // Backup and restore runs publish on the app topic when they start and + // finish, so the run list stays current without a reload. + onMount(() => { + loadData() + return realtime.register(`app:${slug}`, () => loadData({ quiet: true })) + }) - async function loadData() { - loading = true + async function loadData({ quiet = false } = {}) { + if (!quiet) loading = true const [cfgRes, runRes] = await Promise.all([ api.listBackupConfigs(slug), api.listBackupRuns(slug), diff --git a/ui/src/components/__tests__/BackupsTab.test.js b/ui/src/components/__tests__/BackupsTab.test.js index 025ce7ad..61000a94 100644 --- a/ui/src/components/__tests__/BackupsTab.test.js +++ b/ui/src/components/__tests__/BackupsTab.test.js @@ -7,6 +7,14 @@ const apiMock = vi.hoisted(() => ({ detectStrategies: vi.fn(async () => ({ data: [], error: null })), })); vi.mock('../../lib/api.js', () => ({ api: apiMock })); +const realtimeMock = vi.hoisted(() => ({ + handlers: {}, + register: vi.fn((topic, fn) => { + realtimeMock.handlers[topic] = fn; + return () => {}; + }), +})); +vi.mock('../../lib/stores/realtime.svelte.js', () => ({ realtime: realtimeMock })); import BackupsTab from '../BackupsTab.svelte'; @@ -19,6 +27,16 @@ describe('BackupsTab', () => { }); }); + // Runs publish on app: when they start and finish. + it('refetches runs when the app topic fires', async () => { + render(BackupsTab, { slug: 'foo' }); + await waitFor(() => expect(realtimeMock.register).toHaveBeenCalledWith('app:foo', expect.any(Function))); + const before = apiMock.listBackupRuns.mock.calls.length; + apiMock.listBackupRuns.mockResolvedValueOnce({ data: [{ id: 5, status: 'success', started_at: new Date().toISOString() }], error: null }); + await realtimeMock.handlers['app:foo'](); + expect(apiMock.listBackupRuns.mock.calls.length).toBe(before + 1); + }); + it('renders without crashing when there are no configs', async () => { const { container } = render(BackupsTab, { slug: 'foo' }); await waitFor(() => expect(container.firstChild).not.toBeNull());